Fix part of #4374: Add docstrings to Python files. #6013

vibhor98 · 2018-12-20T10:13:28Z

Explanation

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

nithusha21

Hi @vibhor98 thanks for the PR! Here is my first pass. One overall comment, just confirming. Docstrings in the _test files don't need args/return right?

core/domain/feedback_jobs_continuous_test.py

core/domain/learner_playlist_services_test.py

core/domain/user_jobs_one_off_test.py

core/domain/user_services_test.py

nithusha21 · 2018-12-20T11:09:13Z

core/domain/user_services_test.py

@@ -998,6 +1003,7 @@ def utcnow(cls):


 class LastExplorationEditedIntegrationTests(test_utils.GenericTestBase):
+    """Integration tests for the exploration that is edited at the last."""


I am not sure this is right. Could you confirm? I have to look through the code. Will check and let you know in the next review pass.

Suggested change

"""Integration tests for the exploration that is edited at the last."""

"""Integration tests for testing the time the user last edited an exploration updates correctly."""

nithusha21 · 2018-12-20T11:09:21Z

core/domain/user_services_test.py

@@ -1069,6 +1075,7 @@ def test_last_exp_edit_time_gets_updated(self):


 class LastExplorationCreatedIntegrationTests(test_utils.GenericTestBase):
+    """Integration tests for the exploration that is created at the last."""


Suggested change

"""Integration tests for the exploration that is created at the last."""

"""Integration tests for the time the user last created an exploration updates correctly."""

core/jobs_test.py

codecov-io · 2018-12-20T13:10:10Z

Codecov Report

Merging #6013 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #6013   +/-   ##
=======================================
  Coverage     45.3%   45.3%           
=======================================
  Files          520     520           
  Lines        30667   30667           
  Branches      4597    4597           
=======================================
  Hits         13893   13893           
  Misses       16774   16774

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d101331...9926966. Read the comment docs.

nithusha21 · 2018-12-24T06:05:58Z

Hi @vibhor98 please note the lint check failures.

vibhor98 · 2018-12-24T16:20:27Z

Hi @nithusha21, I've resolved lint errors. Please review once again! Thanks!

bansalnitish

Hi @vibhor98, I left some comments for you, PTAL!

bansalnitish · 2018-12-24T17:34:38Z

core/domain/rating_services.py

@@ -57,6 +57,9 @@ def assign_rating_to_exploration(user_id, exploration_id, new_rating):
        raise Exception('Invalid exploration id %s' % exploration_id)

    def _update_user_rating():
+        """Updates the user rating of the exploration. Returns the old rating


I think a single sentence would be better without a break, what do you say? and returns the old rating....

Hi @bansalnitish, I did exactly the same as in your both the comments. But after @nithusha21's review, I made these changes. I think both the ways are acceptable but it's just the matter of individual preferences.

Okay, sounds good!

Typically, we would have a "returns" part of the docstring, I wanted to have it somewhat resemble that. That's why the split sentences. But yeah, I think this does come down to personal preference :P

bansalnitish · 2018-12-24T17:34:59Z

core/domain/stats_jobs_continuous_test.py

        event_services.StartExplorationEventHandler.record(
            exp_id, exp_version, state_name, session_id, {},
            feconf.PLAY_TYPE_NORMAL)

    def _get_calc_output_model(
            self, exploration_id, state_name, calculation_id,
            exploration_version=stats_jobs_continuous.VERSION_ALL):
+        """Gets the StateAnswersCalcOutputModel corresponding to the given


Gets -> Returns ?

Ditto as above.

Gets is fine I believe, and consistent with the function name. That's why I suggested gets :)

nithusha21

Overall looks good. There are a couple of places I have to get back to you about. Will do so soon.

nithusha21 · 2018-12-25T06:51:41Z

core/domain/learner_progress_services_test.py

@@ -106,6 +112,9 @@ def _get_all_completed_collection_ids(self, user_id):
            completed_activities_model else [])

    def _get_all_incomplete_exp_ids(self, user_id):
+        """Returns the list of all the incomplete exploration ids of the learner


This doesn't seem done?

nithusha21 · 2018-12-25T06:53:43Z

core/domain/rating_services.py

@@ -57,6 +57,9 @@ def assign_rating_to_exploration(user_id, exploration_id, new_rating):
        raise Exception('Invalid exploration id %s' % exploration_id)

    def _update_user_rating():
+        """Updates the user rating of the exploration. Returns the old rating


Typically, we would have a "returns" part of the docstring, I wanted to have it somewhat resemble that. That's why the split sentences. But yeah, I think this does come down to personal preference :P

nithusha21 · 2018-12-25T06:54:32Z

core/domain/stats_jobs_continuous_test.py

        event_services.StartExplorationEventHandler.record(
            exp_id, exp_version, state_name, session_id, {},
            feconf.PLAY_TYPE_NORMAL)

    def _get_calc_output_model(
            self, exploration_id, state_name, calculation_id,
            exploration_version=stats_jobs_continuous.VERSION_ALL):
+        """Gets the StateAnswersCalcOutputModel corresponding to the given


Gets is fine I believe, and consistent with the function name. That's why I suggested gets :)

nithusha21 · 2018-12-25T07:04:10Z

core/domain/user_services_test.py

@@ -924,6 +924,7 @@ def test_invalid_subject_interests_are_not_accepted(self):


 class LastLoginIntegrationTests(test_utils.GenericTestBase):
+    """Integration tests for last login by the user."""


Ah, see lines 64-67 in user_services.py. The lastLogin corresponds to the time the user last logged in. So maybe,

Suggested change

"""Integration tests for last login by the user."""

"""Integration tests for testing that the last login time for a user updates correctly."""

nithusha21

LGTM Thanks!

Add docstrings.

873965a

vibhor98 requested a review from nithusha21 December 20, 2018 10:13

nithusha21 suggested changes Dec 20, 2018

View reviewed changes

Made review changes.

165033a

Resolve lint errors.

a7649ea

bansalnitish reviewed Dec 24, 2018

View reviewed changes

nithusha21 reviewed Dec 25, 2018

View reviewed changes

Made requested changes.

9926966

nithusha21 approved these changes Dec 25, 2018

View reviewed changes

nithusha21 merged commit 92c4ecf into oppia:develop Dec 25, 2018

vibhor98 deleted the add-dstrings branch December 25, 2018 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #4374: Add docstrings to Python files. #6013

Fix part of #4374: Add docstrings to Python files. #6013

vibhor98 commented Dec 20, 2018

nithusha21 left a comment

nithusha21 Dec 20, 2018

nithusha21 Dec 25, 2018 •

edited

Loading

vibhor98 Dec 25, 2018

nithusha21 Dec 20, 2018

nithusha21 Dec 25, 2018

vibhor98 Dec 25, 2018

codecov-io commented Dec 20, 2018 •

edited

Loading

nithusha21 commented Dec 24, 2018

vibhor98 commented Dec 24, 2018

bansalnitish left a comment

bansalnitish Dec 24, 2018

vibhor98 Dec 24, 2018

bansalnitish Dec 24, 2018

nithusha21 Dec 25, 2018

bansalnitish Dec 24, 2018

vibhor98 Dec 24, 2018

nithusha21 Dec 25, 2018

nithusha21 left a comment

nithusha21 Dec 25, 2018

nithusha21 Dec 25, 2018

nithusha21 Dec 25, 2018

nithusha21 Dec 25, 2018

vibhor98 Dec 25, 2018

nithusha21 left a comment

		@@ -998,6 +1003,7 @@ def utcnow(cls):


		class LastExplorationEditedIntegrationTests(test_utils.GenericTestBase):
		"""Integration tests for the exploration that is edited at the last."""

	"""Integration tests for the exploration that is edited at the last."""
	"""Integration tests for testing the time the user last edited an exploration updates correctly."""

		@@ -1069,6 +1075,7 @@ def test_last_exp_edit_time_gets_updated(self):


		class LastExplorationCreatedIntegrationTests(test_utils.GenericTestBase):
		"""Integration tests for the exploration that is created at the last."""

	"""Integration tests for the exploration that is created at the last."""
	"""Integration tests for the time the user last created an exploration updates correctly."""

		@@ -924,6 +924,7 @@ def test_invalid_subject_interests_are_not_accepted(self):


		class LastLoginIntegrationTests(test_utils.GenericTestBase):
		"""Integration tests for last login by the user."""

	"""Integration tests for last login by the user."""
	"""Integration tests for testing that the last login time for a user updates correctly."""

Fix part of #4374: Add docstrings to Python files. #6013

Fix part of #4374: Add docstrings to Python files. #6013

Conversation

vibhor98 commented Dec 20, 2018

Explanation

Checklist

nithusha21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithusha21 Dec 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 20, 2018 • edited Loading

Codecov Report

nithusha21 commented Dec 24, 2018

vibhor98 commented Dec 24, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithusha21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithusha21 left a comment

Choose a reason for hiding this comment

nithusha21 Dec 25, 2018 •

edited

Loading

codecov-io commented Dec 20, 2018 •

edited

Loading