Added docstring to "core.domain.exp_services" #2924

DubeySandeep · 2017-01-14T00:35:57Z

Fix #2394: Add useful docstring to "core/domain/exp_services.py"

codecov-io · 2017-01-14T01:11:28Z

Codecov Report

Merging #2924 into develop will decrease coverage by -0.25%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2924      +/-   ##
===========================================
- Coverage    46.38%   46.14%   -0.25%     
===========================================
  Files          226      228       +2     
  Lines        17705    17919     +214     
  Branches      2871     2883      +12     
===========================================
+ Hits          8213     8268      +55     
- Misses        9492     9651     +159

Impacted Files	Coverage Δ
...actions/MathExpressionInput/MathExpressionInput.js	`8.97% <ø> (-6.25%)`	❌
core/templates/dev/head/pages/Base.js	`76.92% <ø> (-3.08%)`	❌
...v/head/pages/collection_player/CollectionPlayer.js	`2.43% <ø> (-2.65%)`	❌
...d/domain/collection/CollectionNodeObjectFactory.js	`91.42% <ø> (-2.52%)`	❌
core/templates/dev/head/pages/profile/Profile.js	`1.31% <ø> (-0.47%)`	❌
...es/exploration_editor/EditorNavigationDirective.js	`2.56% <ø> (-0.22%)`	❌
...ead/domain/exploration/ExplorationObjectFactory.js	`1.72% <ø> (-0.07%)`	❌
...ev/head/pages/exploration_player/PlayerServices.js	`1.7% <ø> (-0.07%)`	❌
...ges/exploration_editor/settings_tab/SettingsTab.js	`0.7% <ø> (-0.05%)`	❌
...head/pages/exploration_editor/ExplorationEditor.js	`3.64% <ø> (-0.03%)`	❌
... and 16 more

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 f7b6572...44d8bf4. Read the comment docs.

seanlip

Hi @DubeySandeep -- I think there might be a misunderstanding about the starting point for this PR. The idea was to start from develop, make the changes that have already been made in this PR, and apply the remaining open comments to get the new PR to a final stage where it can be merged. By starting from square one, you lose all the improvements that were made in the other PR, so I suggest not doing this.

seanlip · 2017-01-16T01:55:48Z

core/domain/exp_services.py

@@ -78,7 +78,7 @@ def _migrate_states_schema(versioned_exploration_states):

    Args:
        versioned_exploration_states: A dict with two keys:
-          - states_schema_version: the states schema version for the
+          - states_schema_version: Int. the states schema version for the


int, not Int

seanlip · 2017-01-16T01:55:54Z

core/domain/exp_services.py

@@ -104,17 +104,21 @@ def _migrate_states_schema(versioned_exploration_states):

 # Repository GET methods.
 def _get_exploration_memcache_key(exploration_id, version=None):
-    """Returns a memcache key for an exploration."""
+    """


Missing one-line summary at top of docstring?

seanlip · 2017-01-16T01:56:13Z

core/domain/exp_services.py

-    """Returns a memcache key for an exploration."""
+    """
+    Args:
+        - exploration_id: str. The exploration id.


Follow format in other files -- there should be no hyphens.

seanlip · 2017-01-16T01:56:58Z

core/domain/exp_services.py

+    Args:
+        - exploration_id: str. The exploration id.
+        - version: int. Version of the exploration , default value None.
+    Returns:


Follow format in other files -- there should be a blank line above this one.

Also, I'm going to stop reviewing here. Please see my general comment -- I think there might be a misunderstanding of what needs to be done here.

I guess you mean to say, i should start working on "develop" branch and make the changes which were asked in the previous PR.

Yes, that's what I meant -- no need to reinvent the wheel :)

Okay! I'm working :) gonna make PR tonight .

seanlip · 2017-01-22T07:46:08Z

Hi @DubeySandeep, have you had a chance to look into this?

526avijitgupta · 2017-02-02T12:21:56Z

@DubeySandeep Are you still working on this ?

DubeySandeep · 2017-02-02T12:33:53Z

I'm done with all the changes which were asked in the PR 2452 but still, I'm going through the docstring as I found it requires some more changes, anyhow I'll make a PR tonight. Thanks. @526avijitgupta

seanlip · 2017-02-06T09:19:42Z

Hi @DubeySandeep, this seems to be dragging on a bit. Any updates?

Thanks!

seanlip · 2017-02-06T20:54:51Z

Reassigning to @kevintab95 for first review pass.

DubeySandeep · 2017-02-06T22:23:15Z

Sorry, but I have made the PR with all the changes which were asked in the previous one. It was late because I am new at git so last week while fetching data from upstream, I changed my working branch to "develop" without making any commit in the previous branch and lost my entire work and then I repeated the same work again. :(
@seanlip

kevintab95 · 2017-02-10T13:29:35Z

core/domain/exp_services.py

+
+    Args:
+        committer_id: str. The id of the user who made the commit.
+        exploration: exploration. The exploration domain object.


Use 'Exploration' not 'exploration' here because you've used it everywhere else.

Okay! Done :)

(Also, note that the reason we use Exploration is because that's what you get when you do type(...) on the argument. It refers to the Exploration class defined in exp_domain.py.)

kevintab95 · 2017-02-10T13:37:54Z

core/domain/exp_services.py

+
+    Args:
+        exp_summary: ExplorationSummary. An ExplorationSummary domain object.
+        user_id: str. ID of the user whose permissions are being checked.


I noticed that you used 'ID' in some places an 'id' in others, can you change them all to just 'id' for consistency? Thanks!

That were written/used by the one who has made the last PR 2452 and that weren't marked for changes so, I left those as they were, but now I'll change them for sure. :)

kevintab95 · 2017-02-10T13:40:23Z

@DubeySandeep I've left some comments, please make the changes. Thanks!

seanlip

This looks pretty good, IMO! I just had a few comments, but hopefully they're easy to address.

seanlip · 2017-02-10T21:57:39Z

core/domain/exp_services.py

+    Returns:
+        ExplorationSummary. The summary domain object corresponding to the
+        given exploration.
+


Drop extra newline.

seanlip · 2017-02-10T21:58:21Z

core/domain/exp_services.py

+
+    Args:
+        exp_ids: list(str). A list of exploration ids of exploration domain
+        objects.


indent this by 4

Done! :)
mistaken* :p

seanlip · 2017-02-10T21:59:09Z

core/domain/exp_services.py

+            exploration is exported.
+
+    Returns:
+        zipfile. A ZIP archive of the exploration.


Did you check the type here (by trying to trigger execution of this function)? Is it a zipfile.ZipFile or a str?

LOG core.domain.exp_services_test.ZipFileExportUnitTests.test_export_to_zip_file: <type 'str'> test_export_to_zip_file (core.domain.exp_services_test.ZipFileExportUnitTests)

So overall this returns 'str' which is then converted into zip via zipfile.ZipFile(StringIO.StringIO(zip_file_output)) which is a zip file. Learnt a lot :) Thanks.

which one would be better?
str. The archive of the exploration.
OR
str. The archive of the exploration which is then converted into zip via zipfile.ZipFile().

str. The contents of the ZIP archive of the exploration (which can be subsequently converted into a zip file via zipfile.ZipFile()).

Note how this docstring does not say anything about what is actually done with the return value, so that the outside context does not "leak into" the function, but it provides context around how the return value could be used by clients (and this acts as a sort of spec for the person writing or modifying code within this function).

seanlip · 2017-02-10T22:00:33Z

core/domain/exp_services.py

+
+    Args:
+        committer_id: str. The id of the user who made the commit.
+        exploration: exploration. The exploration domain object.


(Also, note that the reason we use Exploration is because that's what you get when you do type(...) on the argument. It refers to the Exploration class defined in exp_domain.py.)

seanlip · 2017-02-10T22:01:26Z

core/domain/exp_services.py

+
+    Returns:
+        tuple. A 2-tuple consisting of:
+            -list(ExplorationCommitLogEntry). A list containing


Add a space after each starting hyphen.

seanlip · 2017-02-10T22:03:07Z

core/domain/exp_services.py

+    Returns:
+        tuple. A 2-tuple consisting of:
+            -list(ExplorationCommitLogEntry). A list containing
+                ExplorationCommitlogEntry domain object


domain objects. (note final 's' and full stop).

That said, this doesn't add any info that's not contained in the type at the beginning. Can you have the docstring explain the significance of the return value? E.g. A list of commit log entries corresponding to the given query.

Done!
while going through the return value I came to know that this function returns a 3-tuple the last one is a bool which indicates whether there left any more results after this packet or not. added

even in docstring tuple was misspelled with triple The return value is a triple* Done! :) Thanks.

seanlip · 2017-02-10T22:03:41Z

core/domain/exp_services.py

+            of the exploration.
+
+    Returns:
+        dict.


Add some info about what this dict is used for; what's its context? If I want to add stuff to this dict in the future, for example, what do I need to bear in mind?

seanlip

Looks great! 3 more small things, and then it's ready to merge.

seanlip · 2017-02-17T11:00:49Z

core/domain/exp_services.py

@@ -1490,12 +1497,15 @@ def get_next_page_of_all_non_private_commits(

 def _exp_rights_to_search_dict(rights):
    # Allow searches like "is:featured".
-    """
+    """Returns a search dict with information about the exploration rights. This
+    allows searches like "is:featured".
    Args:


missing newline above this one

seanlip · 2017-02-17T11:01:00Z

core/domain/exp_services.py

    Args:
        rights: ActivityRights. Domain object for the rights/publication status
            of the exploration.

    Returns:
+        dict.  If the status of the given exploration is publicized then it


nit: remove space after "dict."

seanlip · 2017-02-17T11:01:21Z

core/domain/exp_services.py

    Args:
        rights: ActivityRights. Domain object for the rights/publication status
            of the exploration.

    Returns:
+        dict.  If the status of the given exploration is publicized then it
+        returns a dict with a key "is" and value as "featured" else an empty


... with a key "is", and the value "featured". Otherwise, it returns an empty dict.

kevintab95 · 2017-02-17T19:00:14Z

LGTM, great work @DubeySandeep! :)

seanlip

Thanks, @DubeySandeep -- LGTM!

Also, one request for the future: please could you make sure to reply to all comments, following the instructions in step 5 here? In particular, for comments that you've addressed, please reply "Done." -- this helps keep track of which comments have been addressed and which haven't. Thanks!

DubeySandeep · 2017-02-17T23:38:57Z

Sorry, but I thought of not disturbing you all with such silly changes, but now I come to know how much it's important to reply to all comments. I'll do if for sure from next time. :)
Thanks!
@seanlip
@kevintab95
*and now I can work for the assigned bug on a different branch?

seanlip · 2017-02-18T01:08:55Z

Yes, sounds good! Just make sure that your other branch is based off of the latest version of develop -- it's easier if you're working at HEAD, since it reduces the chance of merge conflicts later.

DubeySandeep added 5 commits January 14, 2017 04:20

added docstring to domain/exp_services.py

6f65e86

added docstring to ep_services.py

f61f967

added docstring to ep_services.py

87314f1

added docstring to ep_services.py

a69d5e0

added docstring to ep_services.py

81a6959

DubeySandeep changed the title ~~Docstring~~ Added docstring to "core.domain.exp_services" Jan 14, 2017

seanlip requested changes Jan 16, 2017

View reviewed changes

seanlip mentioned this pull request Jan 25, 2017

Fix #2394: Add useful docstrings to core/domain/exp_services.py following the python style guide #2452

Closed

seanlip assigned DubeySandeep Jan 26, 2017

Made the changes which were asked in PR 2452

54b0599

seanlip requested a review from kevintab95 February 6, 2017 20:54

seanlip assigned kevintab95 and unassigned DubeySandeep Feb 6, 2017

kevintab95 reviewed Feb 10, 2017

View reviewed changes

seanlip requested changes Feb 10, 2017

View reviewed changes

Made the changes which were asked in previous PR

636a22e

seanlip requested changes Feb 17, 2017

View reviewed changes

Made the changes which were asked in previous PR

44d8bf4

kevintab95 approved these changes Feb 17, 2017

View reviewed changes

seanlip approved these changes Feb 17, 2017

View reviewed changes

seanlip merged commit 85e3fcb into oppia:develop Feb 17, 2017

DubeySandeep deleted the docstring branch February 17, 2017 23:32

Added docstring to "core.domain.exp_services" #2924

Added docstring to "core.domain.exp_services" #2924

Conversation

DubeySandeep commented Jan 14, 2017

codecov-io commented Jan 14, 2017 • edited Loading

Codecov Report

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

seanlip commented Jan 22, 2017

526avijitgupta commented Feb 2, 2017

DubeySandeep commented Feb 2, 2017

seanlip commented Feb 6, 2017

seanlip commented Feb 6, 2017

DubeySandeep commented Feb 6, 2017

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

DubeySandeep Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

kevintab95 commented Feb 10, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DubeySandeep Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

DubeySandeep Feb 13, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevintab95 commented Feb 17, 2017

seanlip left a comment

Choose a reason for hiding this comment

DubeySandeep commented Feb 17, 2017

seanlip commented Feb 18, 2017

codecov-io commented Jan 14, 2017 •

edited

Loading

DubeySandeep Feb 10, 2017 •

edited

Loading

DubeySandeep Feb 13, 2017 •

edited

Loading

DubeySandeep Feb 13, 2017 •

edited

Loading