Fixes part of #4374: Update docstrings in the python backend code. #6028

YashJipkate · 2018-12-24T18:18:16Z

Updated docstrings in Python Backend code for core.storage.base_model.gae_models_test, core.storage.base_model.gae_models, core.storage.user.gae_models, core.tests.test_utils, core.tests.performance_tests.base, core.tests.performance_framework.perf_services, core.tests.performance_framework.perf_domain, core.domain.summary_services, core.domain.email_jobs_one_off_test, core.domain.visualization_registry

Explanation

Fixes part of #4374

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.

Updated docstrings in Python Backend code for core.storage.base_model.gae_models_test, core.storage.base_model.gae_models, core.storage.user.gae_models, core.tests.test_utils, core.tests.performance_tests.base, core.tests.performance_framework.perf_services, core.tests.performance_framework.perf_domain, core.domain.summary_services, core.domain.email_jobs_one_off_test, core.domain.visualization_registry

bansalnitish · 2018-12-24T19:18:11Z

HI @YashJipkate, Thanks for the PR.
Could you change the PR title as Fixes part of #4374: Explanation ? Thanks

YashJipkate · 2018-12-24T19:23:41Z

@bansalnitish Done! PTAL!

codecov-io · 2018-12-24T19:23:43Z

Codecov Report

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

@@           Coverage Diff            @@
##           develop    #6028   +/-   ##
========================================
  Coverage    45.21%   45.21%           
========================================
  Files          523      523           
  Lines        30722    30722           
  Branches      4605     4605           
========================================
  Hits         13892    13892           
  Misses       16830    16830

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 cd22391...e048099. Read the comment docs.

vibhor98

Hi @YashJipkate, Took a pass, PTAL!

core/domain/email_jobs_one_off_test.py

core/domain/summary_services.py

core/storage/user/gae_models.py

core/tests/performance_tests/base.py

core/tests/test_utils.py

vibhor98 · 2018-12-24T19:34:59Z

core/tests/test_utils.py

+            headers: dict. Extra headers to send.
+
+        Returns:
+            json_response: webtest.TestResponse instance. The response of the


Remove var name here (json_response)

Also is it a general practice to not mention variable name in the return?
Thanks!
Done!

Oh Thanks for the info!

core/tests/test_utils.py

vibhor98 · 2018-12-24T19:36:10Z

core/tests/test_utils.py

+            upload_files: list. A list of (fieldname, filename, file_content).
+                You can also use just (fieldname, filename) and the file contents
+                will be read from disk.
+            headers: dict. Extra headers to send.


Ditto. Write type info like dict(str, *) for both key-value pairs.

ankita240796

Just a minor nit @YashJipkate, Please fix it. Thanks!

core/tests/performance_framework/perf_services.py

vibhor98

@YashJipkate there are few minor nits that you missed. PTAL, else LGTM! Thanks!

vibhor98 · 2018-12-25T15:46:58Z

core/storage/user/gae_models.py

+        Args:
+            user_id: str. The id of the user.
+            exploration_id: str. The id of the exploration.
+        Returns:


This doesn't seem to be done. Add blank line before Returns

Done! Sorry for missing it.

vibhor98 · 2018-12-25T15:47:20Z

core/storage/user/gae_models.py

@@ -406,6 +418,15 @@ class ExplorationUserDataModel(base_models.BaseModel):

    @classmethod
    def _generate_id(cls, user_id, exploration_id):
+        """Generates key for the instance of ExplorationUserDataModel class in
+        the required format with the arguments provided.
+        Args:


vibhor98 · 2018-12-25T15:47:27Z

core/storage/user/gae_models.py

+        Args:
+            user_id: str. The id of the user.
+            exploration_id: str. The id of the exploration.
+        Returns:


vibhor98 · 2018-12-25T15:47:45Z

core/tests/performance_framework/perf_domain.py

@@ -173,6 +173,16 @@ def get_total_page_size_bytes(self):
        return total_size

    def _get_duration_millisecs(self, event_end, event_initial):
+        """Get page load duration.
+        Args:


vibhor98 · 2018-12-25T15:47:54Z

core/tests/performance_framework/perf_services.py

        # Waits for the complete page to load, otherwise XHR requests
        # made post initial page load will not be recorded.
        time.sleep(time_duration_secs or self.DEFAULT_WAIT_DURATION_SECS)

    def _setup_proxy_server(self, downstream_kbps=None, upstream_kbps=None,
                            latency=None):
+        """Sets up a browsermobproxy server.
+        Args:


vibhor98 · 2018-12-25T15:48:36Z

core/tests/performance_framework/perf_services.py

@@ -294,6 +334,10 @@ def _is_current_user_logged_in(self, driver):
        return True

    def _login_user(self, driver):
+        """Logs in a user and gets the cookie of the current session.
+        Args:


Ditto. Add blank line before Args.

vibhor98 · 2018-12-25T15:49:07Z

core/tests/performance_tests/base.py

@@ -79,6 +103,13 @@ def _record_average_page_timings_from_uncached_session(

    def _record_average_page_timings_from_cached_session(
            self, session_count=DEFAULT_SESSION_SAMPLE_COUNT):
+        """Record average page timings from cached session.


Records (for consistency).

Minor fix

Minor fixes

…ppia into docstrings-backend

Minor fixes Merge branch 'docstrings-backend' of https://github.com/YashJipkate/oppia into docstrings-backend

vibhor98

LGTM, thanks @YashJipkate! 👍

YashJipkate · 2018-12-25T17:30:48Z

@vibhor98 Thanks for your time and guidance and the approval!

YashJipkate · 2018-12-25T18:20:56Z

@ankita240796 Thanks for your review!!

vibhor98 · 2018-12-25T18:59:59Z

Hi @apb7, Can you review this PR also being one of the code owners? Thanks!

apb7

Hi @YashJipkate, this looks good! Left a few comments. Also, can you please ensure that we do not cutoff any line before the 80 character limit? Thanks!

apb7 · 2018-12-25T20:23:57Z

core/domain/email_jobs_one_off_test.py

+            Returns:
+                str. Empty if recipient_id is 'recipient_id2',
+                    None if 'recipient_id1' and 'Email Hash'
+                    otherwise.


Can we shift otherwise to the above line without exceeding the 80 characters limit? If yes, then please go ahead and shift it.

apb7 · 2018-12-25T20:25:10Z

core/storage/base_model/gae_models.py

@@ -429,6 +430,7 @@ def _compute_snapshot(self):
        return self.to_dict(exclude=['created_on', 'last_updated'])

    def _reconstitute(self, snapshot_dict):
+        """Populates the model instance with the snapshot."""


Hi @vibhor98, don't we require Args for snapshot_dict here?

Hi @apb7 I have added it for now; will remove if turns out to be unnecesary.
Also, I am skeptical about the way I wrote the return type. PTAL. Thanks!

It's optional in test files. Else, we need to write them. @YashJipkate, thanks for adding them here!

apb7 · 2018-12-25T20:28:24Z

core/tests/performance_tests/base.py

        self.data_fetcher.load_url(self.page_url)

    def _record_page_metrics_from_uncached_session(self):
+        """Records the page metrics from uncached session for a given page
+        URL.


Can we move URL to the above line as well?

Actually it exceeds the limit in this case...
Thanks!

apb7 · 2018-12-25T20:28:34Z

core/tests/performance_tests/base.py

        self.page_metrics = (
            self.data_fetcher.get_page_metrics_from_uncached_session(
                self.page_url))

    def _record_page_metrics_from_cached_session(self):
+        """Records the page metrics from cached session for a given page
+        URL.


@apb7 Done!!

apb7 · 2018-12-25T20:29:41Z

core/tests/performance_tests/base.py

+        Args:
+            page_config: dict. The page configuration parameters.
+            append_username: bool. Whether to append username to the
+                page URL.


@apb7 Done!!

…ppia into docstrings-backend

linting checks ~2 linting checks ~3

YashJipkate · 2018-12-29T04:49:45Z

@apb7 just a gentle reminder. Kindly review.
Thanks!

apb7

LGTM, thanks @YashJipkate!

vibhor98 · 2018-12-29T13:13:57Z

Hi @BenHenning, can you review this PR being one of the code owners? Thanks!

YashJipkate · 2018-12-31T12:41:15Z

Hi @BenHenning, just a gentle reminder. Kindly review.
Thanks!

YashJipkate · 2019-01-02T11:05:38Z

Hi @BenHenning, A reminder again. Kindly review.
Thanks!

vibhor98 · 2019-01-04T15:53:49Z

Hi @seanlip, can you review this PR once since you're one of the code owners? Thanks!

seanlip

Hi @YashJipkate, thanks for this PR. Just a few comments but otherwise it looks great!

seanlip · 2019-01-04T15:56:37Z

core/tests/performance_framework/perf_domain.py

+        """Get page load duration.
+
+        Args:
+            event_end: str. The event upto which duration measurement is


Nit: upto --> up to

seanlip · 2019-01-04T15:57:20Z

core/tests/performance_framework/perf_services.py

+            downstream_kbps: int. The downstream speed in kbps. Defaults to
+                None.
+            upstream_kbps: int. The upstream speed in kbps. Defaults to None.
+            latency: int. The latency of the server. Defaults to None.


In what unit?

It is in milliseconds.
Done!

seanlip · 2019-01-04T16:05:55Z

core/tests/performance_framework/perf_services.py

+            latency: int. The latency of the server. Defaults to None.
+
+        Returns:
+            tuple(server, proxy): A tuple consisting of the server and the


I don't think "server" and "proxy" are the actual type names; ditto for "driver" below and throughout the remainder of this file. Could you please make sure the types are correct? For instances of classes it should be the class name (so, in this case, it should be Server for the first arg, since it's an instance of browsermobproxy.Server).

Don't just change everything to start with uppercase, though -- make sure to actually figure out what class it is an instance of. When resolving this comment, please also include (in your comment) a reference to where the driver class is defined.

Thanks!

The driver class is defined as here
Changed Server type name and updated proxy type to dict.
PTAL. Thanks!

seanlip · 2019-01-04T16:08:05Z

core/tests/test_utils.py

+            headers: dict(str, *). Extra headers to send.
+
+        Returns:
+            webtest.TestResponse instance: The response of the


webtest.TestResponse. The response of the POST request.

seanlip

LGTM. Thanks!

YashJipkate · 2019-01-05T05:41:08Z

core/tests/test_utils.py

            self, app, url, data, expect_errors=False, expected_status_int=200,
            upload_files=None, headers=None):
+        """Sends a post request with the data provided to the url specified.
+
+        Args:


@seanlip I had to add the disable pylint statement here because it was saying missing-param-doc for parameter "app" but it was clearly there. Also I noticed that changing Args to Params solved the issue but I didnt went ahead with it as it would break the style. PTAL!
Thanks.

YashJipkate · 2019-01-05T05:57:59Z

@nithusha21 @seanlip I just saw it is showing 140 changed files changed. I recently pulled the latest develop branch in my current docstrings-backend branch which has caused this. But the changes in all the files except the 10 I have changed myself are already there in the main develop branch of oppia. I even ran git diff between this branch and the main develop branch of Oppia and found the changes in only the 10 files I modified. I have experienced this same issue earlier and think this is some kind of bug in github's method of displaying diff in the PR where it doesn't compare correctly with the current master branch. PTAL and let me know if there are any suggestions regarding this.

seanlip · 2019-01-05T07:30:03Z

Hi @YashJipkate, I think @apb7 is the person you'll need to talk to for both these issues. The pylint-disable statement should not be added; instead, please try to figure out why missing-param-doc is arising erroneously. If it works correctly for other functions but not for yours, then it might actually be a problem with your code. (In general, don't use pylint pragmas and other similar "escape hatches" to get out of understanding an issue.)

For the 140 files changed, I think this is a dev workflow issue and that @apb7 or @nithusha21 might be able to advise. But, generally, if you follow the steps on the PR instructions page, I believe that this shouldn't happen.

Note that @apb7 is currently travelling so you might want to try and take a stab at resolving these on your own first. For the second one it's fine to create a new PR from scratch if that is easier.

YashJipkate · 2019-01-06T13:25:07Z

core/tests/test_utils.py

-        Params:
-            app: WSGI application. The WSGI application which receives the
+        Args:
+            app: TestApp. The WSGI application which receives the


@seanlip Thanks for your advice! Will keep that in mind next time!
Well thanks to you, I figured out the cause, it was caused since I named the type as WSGI Application and pylint was not happy with the space in between the type. So I set the type to TestApp which is the class as is clear from the piece of code here and also from the source code of TestApp
PTAL. Thanks!

seanlip · 2019-01-06T15:38:38Z

@YashJipkate, the PR looks good, but I noticed you force pushed. We generally discourage that because it overwrites history and makes diffs between successive commits harder to review. Instead perhaps reverting the merge commit (or whatever caused all the new files to be added) might have been a better way to go -- though it might be a good idea to try and figure out how you got into this state in the first place, so that we can avoid this happening to other folks in the future.

Otherwise, the PR looks good, except that it fails Travis lint checks. Did these not run locally on your machine before you pushed?

Once the Travis checks are fixed, this is good to merge. Thanks!

YashJipkate · 2019-01-06T16:01:43Z

@seanlip Thanks for the info. I actually used git reset and then force-pushed my repo (as you saw) at a previous time at a different repo when I encountered this issue, and they were fine with it so I went ahead with it here too.
Wasn't aware of the hardship it causes to reviewers.
And the linting check did run on my local since I used the terminal, don't know how it passed there, might be because my local branch is behind oppia/develop. I have pushed again with the fix.
Thanks!

seanlip · 2019-01-06T16:26:06Z

OK, thanks. No worries.

I'm a bit concerned, though, that this issue seems to be recurring repeatedly for you. If it happens in the future can you document exactly what steps you took locally and file a bug against the dev workflow team so that they can look into it?

YashJipkate · 2019-01-06T16:32:58Z

@seanlip Yeah sure I will keep that in mind the next time I encounter this type of issue.
Thanks!

apb7 · 2019-01-06T17:31:19Z

@YashJipkate, glad that you've figured out the above issues! Also, as Sean suggested, feel free to file an issue if you face any recurring problem. Thanks!

YashJipkate · 2019-01-06T18:02:08Z

@apb7 Sure. Thanks for your help!

YashJipkate · 2019-01-06T18:02:56Z

@seanlip The checks have passed. Please take a final look.
Thanks!

seanlip · 2019-01-06T18:21:46Z

LGTM. Thanks!

YashJipkate requested review from apb7, BenHenning and seanlip as code owners December 24, 2018 18:18

YashJipkate mentioned this pull request Dec 24, 2018

Fix part of #4374 Updated Docstrings in Python Backend Code #6010

Closed

6 tasks

apb7 requested a review from vibhor98 December 24, 2018 18:52

YashJipkate changed the title ~~Fixes part of #4374~~ Fixes part of #4374: Update docstrings in the python backend code. Dec 24, 2018

vibhor98 suggested changes Dec 24, 2018

View reviewed changes

ankita240796 requested changes Dec 24, 2018

View reviewed changes

core/tests/performance_framework/perf_services.py Outdated Show resolved Hide resolved

YashJipkate added 4 commits December 25, 2018 01:50

Review checks

b69d098

Review checks #2

88b9796

Review checks #3

3549f21

Review checks #4

0ee6c7a

vibhor98 suggested changes Dec 25, 2018

View reviewed changes

YashJipkate added 4 commits December 25, 2018 22:00

Review Fixes #5

b1f2063

Minor fix

Review checks #5

1b54f04

Minor fixes

Merge branch 'docstrings-backend' of https://github.com/YashJipkate/o…

e0e00cb

…ppia into docstrings-backend

Review checks #5

7022eae

Minor fixes Merge branch 'docstrings-backend' of https://github.com/YashJipkate/oppia into docstrings-backend

vibhor98 approved these changes Dec 25, 2018

View reviewed changes

ankita240796 approved these changes Dec 25, 2018

View reviewed changes

apb7 reviewed Dec 25, 2018

View reviewed changes

YashJipkate added 3 commits December 26, 2018 02:22

Moved a line up where it can

9814320

Merge branch 'docstrings-backend' of https://github.com/YashJipkate/o…

207c81d

…ppia into docstrings-backend

linting checks

5b85fef

linting checks ~2 linting checks ~3

apb7 approved these changes Dec 29, 2018

View reviewed changes

vibhor98 assigned BenHenning Dec 30, 2018

seanlip reviewed Jan 4, 2019

View reviewed changes

seanlip approved these changes Jan 4, 2019

View reviewed changes

seanlip added the PR: LGTM label Jan 4, 2019

YashJipkate requested a review from nithusha21 as a code owner January 5, 2019 05:31

YashJipkate commented Jan 5, 2019

View reviewed changes

kevinlee12 assigned YashJipkate and unassigned BenHenning Jan 5, 2019

YashJipkate added 2 commits January 6, 2019 18:19

Reviews and linting

aedb60a

Fixed missing params error

91fa9a3

YashJipkate commented Jan 6, 2019

View reviewed changes

Linting checks

e048099

seanlip merged commit 25da701 into oppia:develop Jan 6, 2019

YashJipkate deleted the docstrings-backend branch February 11, 2019 20:00

Fixes part of #4374: Update docstrings in the python backend code. #6028

Fixes part of #4374: Update docstrings in the python backend code. #6028

Conversation

YashJipkate commented Dec 24, 2018 • edited by vibhor98 Loading

Explanation

Checklist

bansalnitish commented Dec 24, 2018

YashJipkate commented Dec 24, 2018

codecov-io commented Dec 24, 2018 • edited Loading

Codecov Report

vibhor98 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

ankita240796 left a comment • edited Loading

Choose a reason for hiding this comment

vibhor98 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

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

vibhor98 left a comment

Choose a reason for hiding this comment

YashJipkate commented Dec 25, 2018 • edited Loading

YashJipkate commented Dec 25, 2018

vibhor98 commented Dec 25, 2018

apb7 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

YashJipkate Dec 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YashJipkate commented Dec 29, 2018

apb7 left a comment

Choose a reason for hiding this comment

vibhor98 commented Dec 29, 2018

YashJipkate commented Dec 31, 2018

YashJipkate commented Jan 2, 2019

vibhor98 commented Jan 4, 2019

seanlip 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

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

YashJipkate Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

YashJipkate commented Jan 5, 2019

seanlip commented Jan 5, 2019

Choose a reason for hiding this comment

seanlip commented Jan 6, 2019

YashJipkate commented Dec 24, 2018 •

edited by vibhor98

Loading

codecov-io commented Dec 24, 2018 •

edited

Loading

ankita240796 left a comment •

edited

Loading

YashJipkate commented Dec 25, 2018 •

edited

Loading

YashJipkate Dec 26, 2018 •

edited

Loading

YashJipkate Jan 5, 2019 •

edited

Loading

YashJipkate commented Jan 6, 2019 •

edited

Loading