Fixes #5843 Changes _check_comment to check for spaces and capitals #5852

ctao5660 · 2018-11-10T23:07:18Z

Explanation

Fixes #5843 , Added two if statements to enforce spaces at beginning of comment and to enforce capital letter at beginning of comment, also updated docstring to reflect the new functionality.

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.

seanlip · 2018-11-11T00:37:50Z

@ctao5660 please could you update the PR title? It should include a summary of what the PR is about.

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: ...".)

lilithxxx

Left some initial comments, PTAL

scripts/pre_commit_linter.py

lilithxxx · 2018-11-11T12:12:30Z

scripts/pre_commit_linter.py

@@ -979,6 +982,26 @@ def _check_comments(all_files):
                        print '%s --> Line %s: %s' % (
                            filename, line_num + 1, message)

+                # Check that comment starts with a space.
+                if line.startswith('#') and len(line) > 1 and line[1] != ' ':


A regex expression might be more suited here.

Done, but if the goal was to reduce length of the conditional statement we haven't saved much length because it still needs to check if the line starts with # and the length is > 1 before it can match the regex because of comments like "#" where the line is only 1 character.

Hi @ctao5660 you can reduce the length of conditional statement by checking all the three conditions with a single regex pattern something like this->'^#[^\s].*$'

Ah, I didn't even think about that. I did that and added another regex for the capital checker as well.

codecov-io · 2018-11-11T22:44:08Z

Codecov Report

Merging #5852 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5852      +/-   ##
===========================================
- Coverage    45.74%   45.73%   -0.01%     
===========================================
  Files          515      516       +1     
  Lines        30136    30147      +11     
  Branches      4533     4535       +2     
===========================================
+ Hits         13785    13788       +3     
- Misses       16351    16359       +8

Impacted Files	Coverage Δ
...v/head/pages/creator_dashboard/CreatorDashboard.js	`18.59% <0%> (-0.47%)`	⬇️
..._editor/feedback_tab/ThreadStatusDisplayService.js	`100% <0%> (ø)`	⬆️
...head/pages/collection_player/CollectionLocalNav.js	`50% <0%> (ø)`

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 7588f62...26ea262. Read the comment docs.

lilithxxx

Made a comment and also look at the checks which are failing. You need to enforce this check to other files in the codebase too.

lilithxxx · 2018-11-12T10:28:53Z

scripts/pre_commit_linter.py

+                    print '%s --> Line %s: %s' % (
+                        filename, line_num + 1, message)
+
+                if not line.startswith('# coding:') and not (


There can be cases like here and here where the comments should begin with a small letter. Look for similar cases and try to find a solution.

Hi @seanlip, some comments can begin with small letter like on the above examples, they are especially keywords. Should those keywords be enclosed between something(maybe?) to avoid confusion and distinguish them and comments can begin with capital or a quote(or whatever we are enclosing it between)?

I think these are manually fixable. For the first one we can replace oppia_tools/ with "The oppia_tools/ folder". For the second, we can remove the typeinfo from the inline comments; it's out of line with other similar classes.

@seanlip In my capital checking regex it's only checking for full words, so things like function names with "_" and "." in them are excluded. Would this be a viable strategy or should I go ahead and manually change all these commented function names like you asked?

I think that sounds fine as well!

Fixed ALL pylint capitalization errors in our codebase that failed Travis CI by hand.

seanlip

It's looking good, thanks @ctao5660! Left a couple of comments.

seanlip · 2018-11-15T02:40:05Z

core/domain/html_validation_service.py

@@ -238,7 +238,7 @@ def convert_to_textangular(html_data):
        elif tag.name == 'td' and tag.next_sibling:
            tag.insert_after(' ')
            tag.unwrap()
-        # div and table rows both are replaced with p tag
+        # Rows div and table both are replaced with p tag


Perhaps: "Divs and table rows are both replaced with ..."

(I don't think "div rows" is a thing.)

seanlip · 2018-11-15T02:40:36Z

core/domain/stats_jobs_one_off.py

@@ -161,7 +161,7 @@ def map(exploration_model):
                            old_state_name = (
                                exp_versions_diff.new_to_old_state_names[
                                    state_name])
-                        # pssm is previous state stats mapping.
+                        # pssm: previous state stats mapping.


'pssm' means 'previous state stats mapping'.

seanlip · 2018-11-15T02:41:15Z

scripts/build_test.py

@@ -61,15 +61,15 @@ def test_minify(self):
        """Tests _minify with an invalid filepath."""
        with self.assertRaises(subprocess.CalledProcessError) as called_process:
            build._minify(INVALID_INPUT_FILEPATH, INVALID_OUTPUT_FILEPATH)
-        # returncode is the exit status of the child process.
+        # .returncode is the exit status of the child process.


Can you use backquotes? E.g.:

`returncode` is the exit status of the child process.

seanlip · 2018-11-15T02:41:22Z

scripts/build_test.py

        self.assertEqual(called_process.exception.returncode, 1)

    def test_minify_and_create_sourcemap(self):
        """Tests _minify_and_create_sourcemap with an invalid filepath."""
        with self.assertRaises(subprocess.CalledProcessError) as called_process:
            build._minify_and_create_sourcemap(
                INVALID_INPUT_FILEPATH, INVALID_OUTPUT_FILEPATH)
-        # returncode is the exit status of the child process.
+        # .returncode is the exit status of the child process.


seanlip · 2018-11-15T02:42:21Z

scripts/pre_commit_linter.py

@@ -979,6 +984,24 @@ def _check_comments(all_files):
                        print '%s --> Line %s: %s' % (
                            filename, line_num + 1, message)

+                # Check that comment starts with a space.


Perhaps explain the significance of #!, too.

seanlip · 2018-11-15T06:58:12Z

core/domain/html_validation_service.py

@@ -238,7 +238,7 @@ def convert_to_textangular(html_data):
        elif tag.name == 'td' and tag.next_sibling:
            tag.insert_after(' ')
            tag.unwrap()
-        # Rows div and table both are replaced with p tag
+        # Divs and table rows both are replaced with p tag


both are --> are both

(Also, if you don't mind, could you please correct the spelling of "apperance" in the line below? I know this was there before, but we might as well fix it. Thanks!)

seanlip · 2018-11-15T06:59:57Z

scripts/pre_commit_linter.py

@@ -984,7 +984,8 @@ def _check_comments(all_files):
                        print '%s --> Line %s: %s' % (
                            filename, line_num + 1, message)

-                # Check that comment starts with a space.
+                # Check that comment starts with a space
+                # and is not a shebang, which doesn't work with a space.


This is still just repeating what a "#!" is. In general, though, when writing comments, try to explain the significance, or "why", of the decisions that are being made in the code.

For example, in this case, you can say something like: "Check that the comment starts with a space, unless it is part of the shebang expression at the start of a bash script." That explains to the reader why that particular case is excluded.

Done I think, TAL

seanlip

All good. Thanks @ctao5660!

seanlip · 2018-11-15T07:49:00Z

@lilithxxx, could you please take a final look at this?

lilithxxx

Thanks @ctao5660, LGTM!

…als (oppia#5852) * added space check and capital check * replaced space checker with regex matche * replaced both space checker and capital checker with regex expressions * fixed capital checker regex to allow special cases * fixed space regex to not catch shebang as error * Fixed pylint errors for comments in entire codebase * code review fixes * added quotes * various spellcheck and clarifications * various spellcheck and clarifications

added space check and capital check

9479acd

ctao5660 requested review from apb7 and seanlip as code owners November 10, 2018 23:07

seanlip assigned apb7 and lilithxxx Nov 11, 2018

ctao5660 changed the title ~~Fix #5843~~ Fixes #5843 Changes _check_comment to check for spaces and capitals Nov 11, 2018

lilithxxx suggested changes Nov 11, 2018

View reviewed changes

replaced space checker with regex matche

ee9d43e

replaced both space checker and capital checker with regex expressions

97fe7b2

lilithxxx suggested changes Nov 12, 2018

View reviewed changes

Chris Tao added 3 commits November 13, 2018 20:57

fixed capital checker regex to allow special cases

ff9f371

fixed space regex to not catch shebang as error

4a702f0

Fixed pylint errors for comments in entire codebase

035ddc5

ctao5660 requested a review from BenHenning as a code owner November 15, 2018 01:07

seanlip requested changes Nov 15, 2018

View reviewed changes

Chris Tao added 2 commits November 14, 2018 22:48

code review fixes

fde70ac

added quotes

84c75b4

seanlip reviewed Nov 15, 2018

View reviewed changes

Chris Tao added 2 commits November 14, 2018 23:13

various spellcheck and clarifications

f994196

various spellcheck and clarifications

26ea262

seanlip approved these changes Nov 15, 2018

View reviewed changes

lilithxxx approved these changes Nov 15, 2018

View reviewed changes

seanlip merged commit 0804599 into oppia:develop Nov 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #5843 Changes _check_comment to check for spaces and capitals #5852

Fixes #5843 Changes _check_comment to check for spaces and capitals #5852

ctao5660 commented Nov 10, 2018 •

edited

seanlip commented Nov 11, 2018

lilithxxx left a comment •

edited

lilithxxx Nov 11, 2018

ctao5660 Nov 11, 2018

lilithxxx Nov 11, 2018

ctao5660 Nov 11, 2018

codecov-io commented Nov 11, 2018 •

edited

lilithxxx left a comment

lilithxxx Nov 12, 2018

lilithxxx Nov 14, 2018

seanlip Nov 14, 2018

ctao5660 Nov 14, 2018 •

edited

seanlip Nov 14, 2018

ctao5660 Nov 15, 2018

seanlip left a comment

seanlip Nov 15, 2018

ctao5660 Nov 15, 2018

seanlip Nov 15, 2018

ctao5660 Nov 15, 2018

seanlip Nov 15, 2018

ctao5660 Nov 15, 2018

seanlip Nov 15, 2018

ctao5660 Nov 15, 2018

seanlip Nov 15, 2018

ctao5660 Nov 15, 2018

seanlip Nov 15, 2018

ctao5660 Nov 15, 2018

seanlip Nov 15, 2018

ctao5660 Nov 15, 2018

seanlip left a comment

seanlip commented Nov 15, 2018

lilithxxx left a comment

Fixes #5843 Changes _check_comment to check for spaces and capitals #5852

Fixes #5843 Changes _check_comment to check for spaces and capitals #5852

Conversation

ctao5660 commented Nov 10, 2018 • edited

Explanation

Checklist

seanlip commented Nov 11, 2018

lilithxxx left a comment • edited

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 Nov 11, 2018 • edited

Codecov Report

lilithxxx 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

ctao5660 Nov 14, 2018 • edited

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

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

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Nov 15, 2018

lilithxxx left a comment

Choose a reason for hiding this comment

ctao5660 commented Nov 10, 2018 •

edited

lilithxxx left a comment •

edited

codecov-io commented Nov 11, 2018 •

edited

ctao5660 Nov 14, 2018 •

edited