Fix part of #7427: Adds args, returns and raises in docstrings of utils.py #8134

Arnesh07 · 2019-12-05T19:31:59Z

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 -m scripts.pre_commit_linter and python -m scripts.run_frontend_tests.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "PROJECT: ..." label (Please add this label for the first-pass review of the PR).
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- 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. Do not only request the review but also add the reviewer as an assignee.

Sync 1

oppiabot · 2019-12-05T19:32:01Z

Hi, @Arnesh07. This pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

Showtim3 · 2019-12-05T19:46:27Z

@Arnesh07 Don't fill the checkboxes that are not valid. @aks681 PTAL!
Thanks

aks681

The main docstrings look fine. Just a couple of distinct comments repeated throughout. PTAL! :)

aks681 · 2019-12-05T20:16:12Z

utils.py

+        filepath: str. A full path to the file.
+
+    Returns:
+        *. Data url created from the filepath of the PNG.


Why is this *? Doesn't this return a string? If so, mention it as str here.

This function returns a call to the function convert_png_binary_to_data_url which has its return type specified like this. Can you make sure whether it is a string so that I can modify docstrings of both the functions? @aks681

aks681 · 2019-12-05T20:17:46Z

utils.py

@@ -398,20 +405,30 @@ def get_time_in_millisecs(datetime_obj):
        datetime_obj: datetime. An object of type datetime.datetime.

    Returns:
-        float. This returns the time in the millisecond since the Epoch.
+        float. This returns the time in the milliseconds since the Epoch.


Drop the 'the' before milliseconds.
Can you edit this comment as well to explain the returned variable? See the below comment regarding it.

aks681 · 2019-12-05T20:18:21Z

utils.py

+    """Returns time in milliseconds since the Epoch.
+
+    Returns:
+        float. This returns the time in the milliseconds since the Epoch.


Ditto.
Also, here you should explain the returned variable itself. So, the comment should be something like 'The time in milliseconds since the Epoch'.

aks681 · 2019-12-05T20:21:29Z

utils.py

+        time_msec: float. Time in milliseconds since the Epoch.
+
+    Returns:
+        str. Returns a string representing the time.


Ditto here. No need to have 'Returns a', since you need to be explaining the returned variable itself here.

aks681 · 2019-12-05T20:22:42Z

utils.py

+        earlier_datetime: datetime. The earlier datetime.
+
+    Returns:
+        bool. Returns true if difference between two datetimes is less than


Ditto, no need of 'Returns' in this line.

aks681 · 2019-12-05T20:24:11Z

utils.py

+
+    Args:
+        base_path: str. The initial path upon which components would be added.
+        path_components: *. Components that would be added to the path.


Whys is this '*'? If it is a list (which seems to be the case), mention that as list(datatype) where datatype is the type of the values in list.

aks681 · 2019-12-05T20:24:36Z

utils.py

+        path_components: *. Components that would be added to the path.
+
+    Returns:
+        str. Returns the path that is obtained after adding the components.


Ditto, drop returns and explain the value itself.

aks681 · 2019-12-05T20:24:50Z

utils.py

+        path: str. Path that is to be normalized.
+
+    Returns:
+        str. Returns path if it is not null else returns a dot string.


Ditto here and elsewhere.

aks681 · 2019-12-05T20:25:49Z

utils.py

@@ -498,6 +538,14 @@ def require_valid_name(name, name_type, allow_empty=False):
        name_type: str. A human-readable string, like 'the exploration title' or
            'a state name'. This will be shown in error messages.
        allow_empty: bool. If True, empty strings are allowed.
+
+    Raises:
+        Exception: Raised when name isn't a string.


Again, no need to have 'Raises' in all these definitions, since the heading is 'Raises'. Just have the exception message itself here and elsewhere.

codecov · 2019-12-05T20:37:31Z

Codecov Report

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

@@           Coverage Diff            @@
##           develop    #8134   +/-   ##
========================================
  Coverage    70.81%   70.81%           
========================================
  Files         1073     1073           
  Lines        63020    63020           
  Branches      4880     4880           
========================================
  Hits         44627    44627           
  Misses       16949    16949           
  Partials      1444     1444

Flag	Coverage Δ
#backend	`100% <ø> (ø)`	⬆️
#frontend	`48.37% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
utils.py	`100% <ø> (ø)`	⬆️

Arnesh07 · 2019-12-08T14:11:58Z

@aks681 I have made the changes, PTAL.

aks681

Almost there! Just 2 more comments.

aks681 · 2019-12-11T11:37:43Z

utils.py

+    """Returns time in milliseconds since the Epoch.
+
+    Returns:
+        float. The time in the milliseconds since the Epoch.


Drop the the before milliseconds.

aks681 · 2019-12-11T11:38:54Z

utils.py

    return python_utils.urllib_unquote(escaped_string).decode('utf-8')


 def get_asset_dir_prefix():
    """Returns prefix for asset directory depending whether dev or prod.
    It is used as a prefix in urls for images, css and script files.
+
+    Returns:
+        str. Prefix '/build' if constants,DEV_MODE is false, otherwise


constants.DEV_MODE

Arnesh07 · 2019-12-11T18:54:49Z

I have made the changes, PTAL. @aks681

aks681

LGTM! Thanks!
Also, as a side note, make sure to address every review comment individually so that we know each of them were addressed.

Arnesh07 · 2019-12-13T14:53:39Z

@kevinlee12 Who would be merging this PR?

kevinlee12 · 2019-12-13T15:41:30Z

I went ahead and merged it!

Arnesh07 · 2019-12-13T15:46:53Z

I went ahead and merged it!

Thank you!

Arnesh07 added 7 commits December 4, 2019 21:53

Merge pull request #1 from oppia/develop

794dc31

Sync 1

Merge branch 'develop' of https://github.com/oppia/oppia into develop

97068b6

Merge branch 'develop' of https://github.com/oppia/oppia into develop

8d4d637

Adds docstrings for utils.py

f42eb0a

Removes trailing whitespace

8d89813

Shortens line length

5929a42

Removes Whitespace oppia#2

e7362dd

Arnesh07 requested a review from aks681 as a code owner December 5, 2019 19:31

Showtim3 added the CHANGELOG: Code health label Dec 5, 2019

Showtim3 assigned aks681 Dec 5, 2019

aks681 suggested changes Dec 5, 2019

View reviewed changes

aks681 assigned Arnesh07 and aks681 and unassigned aks681 Dec 5, 2019

Minor changes

3c1c529

kevinlee12 unassigned Arnesh07 Dec 10, 2019

aks681 suggested changes Dec 11, 2019

View reviewed changes

aks681 assigned Arnesh07 Dec 11, 2019

Minor changes

1320f47

aks681 approved these changes Dec 12, 2019

View reviewed changes

kevinlee12 merged commit f773c08 into oppia:develop Dec 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #7427: Adds args, returns and raises in docstrings of utils.py #8134

Fix part of #7427: Adds args, returns and raises in docstrings of utils.py #8134

Arnesh07 commented Dec 5, 2019 •

edited

Loading

oppiabot bot commented Dec 5, 2019

Showtim3 commented Dec 5, 2019

aks681 left a comment

aks681 Dec 5, 2019

Arnesh07 Dec 6, 2019

Arnesh07 Dec 7, 2019

aks681 Dec 5, 2019

aks681 Dec 5, 2019

aks681 Dec 5, 2019

aks681 Dec 5, 2019

aks681 Dec 5, 2019

aks681 Dec 5, 2019

aks681 Dec 5, 2019

aks681 Dec 5, 2019

codecov bot commented Dec 5, 2019 •

edited

Loading

Arnesh07 commented Dec 8, 2019

aks681 left a comment

aks681 Dec 11, 2019 •

edited

Loading

aks681 Dec 11, 2019

Arnesh07 commented Dec 11, 2019

aks681 left a comment

Arnesh07 commented Dec 13, 2019

kevinlee12 commented Dec 13, 2019

Arnesh07 commented Dec 13, 2019

Fix part of #7427: Adds args, returns and raises in docstrings of utils.py #8134

Fix part of #7427: Adds args, returns and raises in docstrings of utils.py #8134

Conversation

Arnesh07 commented Dec 5, 2019 • edited Loading

Explanation

Checklist

oppiabot bot commented Dec 5, 2019

Showtim3 commented Dec 5, 2019

aks681 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

codecov bot commented Dec 5, 2019 • edited Loading

Codecov Report

Arnesh07 commented Dec 8, 2019

aks681 left a comment

Choose a reason for hiding this comment

aks681 Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arnesh07 commented Dec 11, 2019

aks681 left a comment

Choose a reason for hiding this comment

Arnesh07 commented Dec 13, 2019

kevinlee12 commented Dec 13, 2019

Arnesh07 commented Dec 13, 2019

Arnesh07 commented Dec 5, 2019 •

edited

Loading

codecov bot commented Dec 5, 2019 •

edited

Loading

aks681 Dec 11, 2019 •

edited

Loading