Fixes #7010: Adds a linting check that ensures all TODO comments have a username with them #7114

teddymarchildon · 2019-07-08T22:59:57Z

Fixes #7010: Adds a linting check that ensures all TODO comments have a username with them.

This adds a lint check that ensures all TODO comments have a username with them.

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 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.

teddymarchildon · 2019-07-08T23:02:59Z

I'm having a hard time adding reviewers (not quite sure why) but @seanlip @apb7 @lilithxxx PTAL whenever you get a chance! Thanks so much.

DubeySandeep · 2019-07-09T01:24:49Z

Hi @teddymarchildon, I've assigned @apb7 as a reviewer for this PR! (and sorry for the trouble!)

Also, can you please check why the lint tests are failing for this PR and fix it?

teddymarchildon · 2019-07-09T02:13:06Z

It looks like it is failing the linter because this PR adds a lint check to ensure that TODO comments are assigned to a user. I thought the scripts/pre_commit_linter.py only is run on the files changed in the commit, but it looks like it is run on the entire codebase in the travis-ci. So, for example, core/domain/story_services_test line 1358 has a TODO comment without an assigned user, but that file is not changed in this PR.

Should I find the owner, or the last person who edited the TODO comment, of the TODO comments currently unassigned and write them in as the assigned user? It looks like the vast majority of TODO comments already have an assigned user.

DubeySandeep · 2019-07-09T02:31:11Z

I thought the scripts/pre_commit_linter.py only is run on the files changed in the commit

Locally (as git push hook) it runs on the files changed in the feature-branch. But on travis we run these tests for the entire repo.

Should I find the owner, or the last person who edited the TODO comment, of the TODO comments currently unassigned and write them in as the assigned user?

Yeah, this sounds good to me! Also, in case you'll find it hard to assign a username for any TODO item than you can assign it to the code-owner of the file.

(For issues like #7010 we expect the contributor to introduce the new check as well as fix the existing issues. )

scripts/pre_commit_linter.py

…S, and PY files

…ner of the file

oppiabot · 2019-07-09T18:12:42Z

Hi @teddymarchildon. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

codecov · 2019-07-09T18:12:43Z

Codecov Report

Merging #7114 into develop will decrease coverage by 0.23%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #7114      +/-   ##
===========================================
- Coverage    98.06%   97.82%   -0.24%     
===========================================
  Files          377      376       -1     
  Lines        63208    62757     -451     
===========================================
- Hits         61987    61395     -592     
- Misses        1221     1362     +141

Impacted Files	Coverage Δ
core/controllers/cron.py	`36.26% <0%> (-63.74%)`	⬇️
core/domain/cron_services.py	`36.84% <0%> (-63.16%)`	⬇️
core/controllers/reader.py	`81.47% <0%> (-18.53%)`	⬇️
core/controllers/reader_test.py	`100% <0%> (ø)`	⬆️
main.py	`87.61% <0%> (ø)`	⬆️
core/controllers/pages.py	`100% <0%> (ø)`	⬆️
core/controllers/pages_test.py	`100% <0%> (ø)`	⬆️
main_cron.py
core/controllers/cron_test.py
__init__.py	`100% <0%> (ø)`
... and 1 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 05cff06...6ee8e38. Read the comment docs.

codecov · 2019-07-09T18:12:44Z

Codecov Report

Merging #7114 into develop will not change coverage.
The diff coverage is 95.12%.

@@           Coverage Diff           @@
##           develop   #7114   +/-   ##
=======================================
  Coverage     87.5%   87.5%           
=======================================
  Files          945     945           
  Lines        83414   83414           
  Branches      2349    2349           
=======================================
  Hits         72993   72993           
  Misses        9976    9976           
  Partials       445     445

Impacted Files	Coverage Δ
...plates/dev/head/domain/story/StoryObjectFactory.ts	`100% <ø> (ø)`	⬆️
...v/head/domain/state_card/StateCardObjectFactory.ts	`80.45% <ø> (ø)`	⬆️
...in/exploration/WrittenTranslationsObjectFactory.ts	`64.7% <ø> (ø)`	⬆️
...domain/suggestion/SuggestionThreadObjectFactory.ts	`93.75% <ø> (ø)`	⬆️
...dev/head/domain/exploration/StatesObjectFactory.ts	`84.21% <ø> (ø)`	⬆️
...head/domain/statistics/PlaythroughObjectFactory.ts	`100% <ø> (ø)`	⬆️
...ev/head/domain/story/StoryContentsObjectFactory.ts	`88.08% <ø> (ø)`	⬆️
...ead/domain/exploration/ExplorationObjectFactory.ts	`55.26% <ø> (ø)`	⬆️
core/domain/story_services_test.py	`100% <ø> (ø)`	⬆️
...ates/dev/head/domain/user/UserInfoObjectFactory.ts	`100% <ø> (ø)`	⬆️
... and 60 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 d5f4315...b459568. Read the comment docs.

…mment-lint-check

apb7

Hi @teddymarchildon, thanks for the PR. Left some comments, ptal.

core/domain/story_services_test.py

apb7 · 2019-07-10T12:25:06Z

scripts/pre_commit_linter.py

@@ -220,6 +220,13 @@
        'excluded_files': (),
        'excluded_dirs': ('core/tests/')
    },
+    {
+        'regexp': r'\/\/\sTODO[^\(]*[^\)]$',


A couple of points here:

I think it would be better if we put the regexp in BAD_PATTERNS since this needs to be checked for all files, irrespective of the type.

Let's enforce the TODO(@username): pattern instead of TODO(username):.

Gotcha, will enforce the @ sign!

I actually thought about putting the regexp in BAD_PATTERNS, but all of those patterns are checked here as just seeing if the pattern exists in the file, not with a regular expression. I would be happy to find a pattern that works for that method if you prefer! Something like TODO: as a bad pattern

@teddymarchildon, I see. Can we set up a BAD_PATTERNS_REGEXP for this (since we have bad patterns regexp for different file types)? We can then use the same mechanism. Thanks!

Hi @teddymarchildon, have you looked into this? Thanks!

Ah I must've missed that comment, working on it now!

@apb7 Updated! Much cleaner - thanks for the heads up.

scripts/pre_commit_linter.py

kevinlee12 · 2019-07-18T18:19:38Z

Github was having issues with their API earlier, which might be why.

oppiabot · 2019-07-19T08:45:02Z

Hi @teddymarchildon. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

apb7

LGTM! Thanks, @teddymarchildon. Nice work!

DubeySandeep

@teddymarchildon, This looks great, thanks for adding the checks and fixing the existing wrong patterns :)

I've left a few comments, PTAL.

@apb7: Can we have a follow-up issue for handling: (We can implement this latter)

TODO comments end with a period(.) [I think we have such checks for comments]
TODO message should start with a capital letter.

DubeySandeep · 2019-07-19T20:59:13Z

scripts/pre_commit_linter.py

+BAD_PATTERNS_REGEXP = [
+    {
+        'regexp': r'TODO[^\(]*[^\)][^:]*[^\w]*$',
+        'message': 'Please assign TODO comments to a user ' +


Do we need + here?

@apb7, do we want these test to only fail for a specific pattern or for any unexpected TODO pattern?

@DubeySandeep The + sign is used so that each line length is below 80 characters as mandated by pylint

I'm happy to enforce the uppercase starting character in this regexp, and I believe all comments have to end with a period as enforced by one of the linting processes

Hi @DubeySandeep, thanks for the comments. Please find the responses inline:

do we want these test to only fail for a specific pattern or for any unexpected TODO pattern?

I think the scope of this PR is to ensure that all TODO comments have an associated username in a specific pattern. Let me know if you think otherwise.

Do we need + here?

Nice catch! No, I don't think we require + here. @teddymarchildon, can you please check other multi-line bad pattern messages. They do not use +.

TODO comments end with a period(.) [I think we have such checks for comments]

I think the comment check will cover this as well, right?

TODO message should start with a capital letter.

I'll file a follow-up issue for this (filed #7182). @teddymarchildon, if you're interested, please let us know on that issue thread. We'll assign it to you. Thanks!

Thanks for the reply @apb7, @teddymarchildon can you please update the PR as Apurv suggested so that we can quickly merge this PR?

@DubeySandeep I believe I have addressed the comments, and can start working on the next issue once this one is merged. Let me know if there is anything else to fix up or if I missed something from those comments!

DubeySandeep · 2019-07-19T21:21:13Z

scripts/pre_commit_linter.py

+                    if _check_bad_pattern_in_file(
+                            filepath, file_content, regexp):
+                        failed = True
+                        total_error_count += 1


@apb7, total_error_count is the number of files with bad patterns? (that's also not true though) or it's the number of failed patterns?

(I mean in one file there can be multiple errors for a single bad_pattern regexp.)

I think it's the total number of failed patterns (for reference, please see other regexp checks). Thanks!

DubeySandeep · 2019-07-20T16:26:59Z

core/templates/dev/head/pages/get-started-page/get-started-page.mainpage.html

@@ -46,7 +46,7 @@ <h2 translate="I18N_GET_STARTED_PAGE_PARAGRAPH_8_HEADING"></h2>
            <p translate="I18N_GET_STARTED_PAGE_PARAGRAPH_8">
            </p>

-            <!-- todo: make it easy to get the right link to share, update this doc with that info -->
+            <!-- TODO(vojtechjelinek): make it easy to get the right link to share, update this doc with that info -->


@apb7, I think the "Period at the end of comment" doesn't work here!

DubeySandeep · 2019-07-20T16:27:25Z

core/tests/protractor_desktop/extensions.js

@@ -48,7 +48,7 @@ describe('rich-text components', function() {
    explorationEditorMainTab.setContent(function(richTextEditor) {
      richTextEditor.appendBoldText('bold');
      richTextEditor.appendPlainText(' ');
-      // TODO (Jacob) add test for image RTE component
+      // TODO(Jacob): add test for image RTE component


@apb7, Ditto.

DubeySandeep · 2019-07-20T16:28:33Z

core/tests/protractor_desktop/navigation.js

@@ -95,11 +95,11 @@ describe('Oppia static pages tour', function() {

  afterEach(function() {
    general.checkForConsoleErrors([
-      // TODO (Jacob) Remove when
+      // TODO(Jacob): Remove when
      // https://code.google.com/p/google-cast-sdk/issues/detail?id=309 is fixed


@apb7, I see this in multiple places, ca we add this also as a part of #7182?

seanlip · 2019-07-23T04:10:35Z

I'm a little confused about what the next step is here. I see some comments by @DubeySandeep addressed to @apb7 and I don't know whether the ball is still in @teddymarchildon's court or not.

I'm assigning @apb7 to reply to @DubeySandeep's comments. @teddymarchildon -- would you mind replying to this thread and leaving a top-level comment if you're done with everything and would just like to get a review? I'm happy to merge this once @apb7 and @DubeySandeep confirm.

Thanks!

teddymarchildon · 2019-07-23T04:41:59Z

My bad, I forgot to add a comment once I was done addressing comments. I just added one replying, but I think I have addressed the issues @DubeySandeep and @apb7 brought up - if not let me know and I can fix it up!

I can start working on the next issue once this one is merged.

seanlip · 2019-07-23T05:57:03Z

Great, thanks @teddymarchildon! In that case, deassigning you and adding @DubeySandeep @apb7 for final review/confirmation.

Also, please note that you can work on more than one issue at a time! Just make sure to do that on a new, separate feature branch created from develop, as described here.

DubeySandeep

I think the issue stated in #7010 is resolved and we can merge this PR. LGTM!

DubeySandeep · 2019-07-23T07:18:26Z

@seanlip, Can we merge this PR directly (As stated here)?

Note: @apb7 has already approved this PR!

DubeySandeep · 2019-07-23T07:19:28Z

@teddymarchildon, Congrats on your first successful PR to Oppia! :)

seanlip · 2019-07-23T07:31:25Z

OK, merging! Congrats @teddymarchildon :D

tmarchildon added 2 commits July 8, 2019 16:26

Add linting function to check that TODO comments have a user

4e0a755

Fix regex to match digits present in usernames

caa0297

teddymarchildon requested a review from apb7 as a code owner July 8, 2019 22:59

teddymarchildon changed the title ~~Todo comment lint check~~ Fixes #7010: Adds a linting check that ensures all TODO comments have a username with them Jul 8, 2019

DubeySandeep assigned apb7 Jul 9, 2019

kevinlee12 added the CHANGELOG: Code health label Jul 9, 2019

DubeySandeep reviewed Jul 9, 2019

View reviewed changes

scripts/pre_commit_linter.py Outdated Show resolved Hide resolved

tmarchildon added 4 commits July 9, 2019 10:41

Put the bad regex for todo comments in the global variables for TS, J…

43cd237

…S, and PY files

Add regex to HTML bad patterns, and fix TODO comments with the codeow…

a64dc31

…ner of the file

Remove unnecessary comment

560f63e

Shorten line lengths in comments

6ee8e38

teddymarchildon requested review from aks681, kevinlee12 and vojtechjelinek as code owners July 9, 2019 18:12

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Jul 9, 2019

Fix conflict in the pre_commit_linter

ca14b12

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jul 9, 2019

tmarchildon added 2 commits July 9, 2019 14:02

Add a user to a TODO comment in the jobs.py file

cff13bd

Merge branch 'develop' of https://github.com/oppia/oppia into todo-co…

b83bf46

…mment-lint-check

teddymarchildon requested a review from seanlip as a code owner July 9, 2019 20:19

Fix TODO comment in extensions

f07882d

apb7 reviewed Jul 10, 2019

View reviewed changes

apb7 removed their assignment Jul 10, 2019

Add a message for sean's todo

810f412

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Jul 19, 2019

apb7 assigned teddymarchildon Jul 19, 2019

tmarchildon added 2 commits July 19, 2019 10:19

Put the TODO regexp in its own structure, and enforce : and message

bfa8731

Fix merge conflict with develop

e950870

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jul 19, 2019

apb7 approved these changes Jul 19, 2019

View reviewed changes

vojtechjelinek removed request for prasanna08 and ankita240796 July 19, 2019 19:37

Fix more TODO comment formats

5417d21

DubeySandeep reviewed Jul 19, 2019

View reviewed changes

apb7 assigned DubeySandeep Jul 20, 2019

DubeySandeep removed their assignment Jul 20, 2019

DubeySandeep reviewed Jul 20, 2019

View reviewed changes

Remove unnecessary + sign from message

b459568

seanlip assigned apb7 Jul 23, 2019

seanlip assigned DubeySandeep and unassigned teddymarchildon Jul 23, 2019

DubeySandeep approved these changes Jul 23, 2019

View reviewed changes

seanlip merged commit d89d68f into oppia:develop Jul 23, 2019

Fixes #7010: Adds a linting check that ensures all TODO comments have a username with them #7114

Fixes #7010: Adds a linting check that ensures all TODO comments have a username with them #7114

Conversation

teddymarchildon commented Jul 8, 2019 • edited by kevinlee12 Loading