Fix part of #3905: Add lint checks for alphabetical import order #4343

apb7 · 2018-01-03T11:45:46Z

This PR enables pre_commit_linter.py to check for alphabetical import order. Also, import order for all files have been fixed.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
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.

apb7 · 2018-01-03T11:46:51Z

@seanlip and @DubeySandeep: Please review!
Thanks!

seanlip

Good start, thanks @apb7! Took a first pass.

seanlip · 2018-01-03T17:59:11Z

constants_test.py

@@ -16,8 +16,9 @@

 import os

-import constants #pylint: disable=relative-import
-from core.tests import test_utils #pylint: disable=relative-import
+import constants  # pylint: disable=relative-import


This check for two spaces is good! Does it come with isort too (i.e. will it error if it's wrong)?

Yes! It comes with isort. It is the default setting though I will look into it.

seanlip · 2018-01-03T17:59:29Z

core/controllers/admin.py

@@ -36,6 +34,7 @@
 from core.domain import user_services
 from core.platform import models
 import feconf
+import jinja2


jinja2 is actually an App Engine library, not part of our codebase. It should be considered third-party.

Same for pipeline, webapp2, webtest and yaml, I think.

Okay! I will put them on the third-party list.

seanlip · 2018-01-03T18:01:32Z

core/controllers/editor.py

@@ -16,15 +16,11 @@

 """Controllers for the editor view."""

+import StringIO


It seems a bit odd to me to have uppercase above lowercase when sorting, instead of treating this similar to stringIO. Is there a setting that can be used to ignore case? If not, that's fine.

I could not find a way around this. There was no setting pertaining to this.

seanlip · 2018-01-03T18:01:53Z

core/controllers/editor.py

 import feconf
+import jinja2
+import mutagen


Same thing -- jinja2, mutagen are 3rd-party

Okay! I'll add them!

seanlip · 2018-01-03T18:02:22Z

core/controllers/editor_test.py

 from core.domain import user_services
 from core.platform import models
-from core.platform.taskqueue import gae_taskqueue_services as taskqueue_services
+from core.platform.taskqueue import \


Nope -- never end lines with . See our coding guidelines. The previous way was fine, I think (and, if not, a different workaround is needed).

Okay, got it! I will change the length of import lines according to our coding guidelines.

seanlip · 2018-01-03T18:08:11Z

core/tests/gae_suite.py


 import feconf

+#pylint: enable=wrong-import-order


Well... if it works without this then we don't need this at all, right?

Also, more generally, please make sure to verify all files manually after the changes have been made by isort.

I will see this and manually check the files.

seanlip · 2018-01-03T18:08:20Z

core/tests/performance_framework/perf_services.py

@@ -20,11 +20,10 @@
 import time
 import urlparse

+import browsermobproxy


seanlip · 2018-01-03T18:08:29Z

extensions/classifiers/LDAStringClassifier/LDAStringClassifier.py


 from core.domain.base_classifier import BaseClassifier
 import feconf
+import numpy


Okay, got it!

seanlip · 2018-01-03T18:09:10Z

main.py


 from mapreduce import main as mapreduce_main
 from mapreduce import parameters as mapreduce_parameters
 import webapp2
 from webapp2_extras.routes import RedirectRoute

+# pylint: enable=relative-import


Check whether this is still needed, please.

Ditto elsewhere. Make sure to manually audit all files and drop pragmas that are no longer needed.

Sure! I will ensure this.

Why is this still here? Ditto elsewhere.

It used to exist because of mapreduce/webapp2 I think, but if that's not needed any more then I'm not sure what it's point is. Could you clarify?

Also, make sure to run the app locally (plus at least one backend test) to ensure all of the infrastructure still works after your changes.

I tried removing it and then running the lint checks. It produces an error as Pylint recommends import .feconf instead of import feconf. It is not because of mapreduce/webapp2.

Ah ok. I'm fine with import .feconf, though it's a bit unfamiliar. Does from . import feconf work? If so, that would probably be better.

Also, if we change it here, we should change it everywhere, for consistency.

Okay, so this is the error produced in main.py:

I think it will difficult to do away with pylint: enable=relative-import as we will have to change it manually for each such import.
Please suggest what should be done. We can also disable relative import check from .pylintrc and then remove these statements.
Thanks!

I see, interesting. OK, let's go with the enable-relative-import pragma then, as we had it before. Thanks for checking!

seanlip · 2018-01-03T18:11:14Z

scripts/pre_commit_linter.py

    newline_messages = _check_newline_character(all_files)
    linter_messages = _pre_commit_linter(all_files)
    pattern_messages = _check_bad_patterns(all_files)
    all_messages = (
        linter_messages + newline_messages + pattern_messages)
    if any([message.startswith(_MESSAGE_TYPE_FAILED) for message in
-            all_messages]):
+            all_messages]) or import_order_message:


why not just add import_order_message to all_messages? Also keep the order in line 663 the same as the order in lines 658-661 for consistency.

import_order_message returns boolean and not the actual message. Therefore I used it this way. I will ensure the consistency though.

seanlip · 2018-01-03T18:20:16Z

Hey @apb7, one tip: instead of replying to comments one-by-one, go to the "Files Changed" tab and make comments inline, choosing "Start a review" as you do so. Then GitHub will send a single email once you've made all the comments.

Thanks!

apb7 · 2018-01-03T18:36:50Z

Sorry, @seanlip! I will take care of this from the next time. Also, thanks for such an in-depth review!

codecov-io · 2018-01-03T20:59:58Z

Codecov Report

Merging #4343 into develop will increase coverage by 0.92%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4343      +/-   ##
===========================================
+ Coverage    43.74%   44.66%   +0.92%     
===========================================
  Files          375      375              
  Lines        23409    23038     -371     
  Branches      3778     3663     -115     
===========================================
+ Hits         10241    10291      +50     
+ Misses       13168    12747     -421

Impacted Files	Coverage Δ
...dev/head/domain/utilities/BrowserCheckerService.js	`72.22% <0%> (-22.52%)`	⬇️
...ead/services/contextual/WindowDimensionsService.js	`60% <0%> (-20%)`	⬇️
...ead/domain/exploration/ExplorationObjectFactory.js	`58.46% <0%> (-4.04%)`	⬇️
...er/RefresherExplorationConfirmationModalService.js	`4.76% <0%> (-2.39%)`	⬇️
...s/dev/head/domain/utilities/LanguageUtilService.js	`86.11% <0%> (-1.07%)`	⬇️
...d/pages/exploration_player/ProgressNavDirective.js	`2.22% <0%> (-0.72%)`	⬇️
...ions/MusicNotesInput/directives/MusicNotesInput.js	`53.96% <0%> (-0.49%)`	⬇️
...n/collection/CollectionPlaythroughObjectFactory.js	`4.34% <0%> (-0.42%)`	⬇️
.../templates/dev/head/services/AudioPlayerService.js	`17.24% <0%> (-0.31%)`	⬇️
...ges/exploration_editor/editor_tab/StateSolution.js	`1.25% <0%> (-0.14%)`	⬇️
... and 37 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 3ae597b...c8e8535. Read the comment docs.

apb7 · 2018-01-03T21:16:03Z

Hey @seanlip, I confirmed the isort documentary and it does check for two lines after top-level class or function, by default. Also, I have removed #pylint: enable=wrong-import-order wherever possible but we cannot remove #pylint: enable=relative-import yet as it causes linting errors.
Can you please review it again?
Thanks!

seanlip · 2018-01-04T01:19:55Z

.isort.cfg

+line_length=80
+force_single_line=true
+force_sort_within_sections=true
+known_third_party=google.appengine,mapreduce,webapp2,webapp2_extras,jinja2,pipeline,webtest,yaml,browsermobproxy,numpy,mutagen,cloudstorage


Suggest alphabetizing this list.

I will do so.

seanlip · 2018-01-04T01:21:01Z

core/controllers/learner_dashboard_test.py

@@ -20,6 +20,7 @@
 from core.tests import test_utils
 import feconf

+


OK -- just to confirm, let's say you make this just have one line, then run the pre-commit script (but no autofixing). Will this show up as an error? What does the error message say?

seanlip · 2018-01-04T01:21:36Z

core/platform/email/mailgun_email_services.py

 from core.platform.email import gae_email_services
 import feconf
+import requests


requests is 3rd-party. There are other third-party libs I'm seeing too that you haven't detected, so, again, please check this whole PR carefully.

Okay, sure!

@seanlip: I have added requests to the known third-party list. I will try to add others. Can you please list others which have been missed?
Thanks!

I think I mentioned before how to check this, but maybe I wasn't clear enough. Stuff like import feconf and import utils is fine because, at the top level of the codebase, there are feconf.py and utils.py. But import cloudstorage isn't fine because there's no cloudstorage.py at the top-level of the codebase, so that's a third-party lib. That's how you can check.

Thank you @seanlip! I will see this. I have included all the libraries which were being installed by the the bash scripts for now. Thanks!

seanlip · 2018-01-04T01:22:35Z

main.py


 from mapreduce import main as mapreduce_main
 from mapreduce import parameters as mapreduce_parameters
 import webapp2
 from webapp2_extras.routes import RedirectRoute

+# pylint: enable=relative-import


Why is this still here? Ditto elsewhere.

It used to exist because of mapreduce/webapp2 I think, but if that's not needed any more then I'm not sure what it's point is. Could you clarify?

Also, make sure to run the app locally (plus at least one backend test) to ensure all of the infrastructure still works after your changes.

seanlip · 2018-01-04T01:27:02Z

scripts/pre_commit_linter.py

+        any(fnmatch.fnmatch(filename, pattern) for pattern in EXCLUDED_PATHS)]
+    failed = False
+    for filename in all_files:
+        if isort.SortImports(filename, check=True).incorrectly_sorted:


Just to confirm: does this line update the files with wrong import order as well, or does it just return True/False and do nothing else?

This line prints the actual error message with the file path and returns True if it finds an error else False.

Ah, that seems very side-effecty. Please add a comment above this line explaining that, since it's rather unexpected.

seanlip · 2018-01-04T01:27:23Z

scripts/pre_commit_linter.py

+    print '----------------------------------------'
+    print ''
+    if failed:
+        summary_message = '%s   Import order failed' % _MESSAGE_TYPE_FAILED


This isn't really useful for someone trying to debug and fix a lint error...? You'll need to give more information, including perhaps a list of files (and errors, if possible).

Also, here and below, the message should be "Import order checks failed"

This is just to maintain consistency with the rest of the checks so that import order messages can be incorporated in all messages. The actual error message is printed automatically by Line 649 when SortImports is called.
Thanks!

apb7 · 2018-01-06T13:26:17Z

@seanlip and @DubeySandeep: I am unable to understand why Travis build is failing in case of bash scripts/run_e2e_tests.sh --suite=misc --prod_env. It passes all tests when I run it locally.
Thanks!

nithusha21 · 2018-01-06T13:47:34Z

Hi @apb7 I have restarted the test on Travis and it passes. Sometimes Travis tests are flaky and if you feel that the issue is not because of your code changes and the test seems to be failing let us know on the Gitter chat and one of the contributors will restart it for you.

apb7 · 2018-01-06T14:38:49Z

Thank you @nithusha21!
@seanlip: Can you please review it? Thanks!

seanlip

I think it looks good. Thanks @apb7!

apb7 · 2018-01-06T19:45:18Z

Thank you @seanlip!

apb7 added 2 commits January 3, 2018 16:12

Fixes part of #3905

830b533

Corrected import order

81460c6

DubeySandeep requested a review from seanlip January 3, 2018 15:03

DubeySandeep assigned seanlip Jan 3, 2018

DubeySandeep changed the title ~~Fixes part of #3905: Add lint checks for alphabetical import order~~ Fix part of #3905: Add lint checks for alphabetical import order Jan 3, 2018

seanlip reviewed Jan 3, 2018

View reviewed changes

Files corrected

7cf497a

set check to True

4697c7b

seanlip reviewed Jan 4, 2018

View reviewed changes

apb7 added 3 commits January 4, 2018 21:21

Merge branch 'develop' into import-patch

f9fe2d4

Review changes made

18f652b

Updated branch

c8e8535

seanlip approved these changes Jan 6, 2018

View reviewed changes

seanlip added the PR: LGTM label Jan 6, 2018

seanlip merged commit 60067ed into oppia:develop Jan 6, 2018

seanlip mentioned this pull request Jan 7, 2018

Add lint checks for common coding issues arising in code reviews. #3905

Closed

34 tasks

apb7 deleted the import-patch branch January 7, 2018 09:35

		@@ -16,15 +16,11 @@

		"""Controllers for the editor view."""

		import StringIO

		@@ -20,6 +20,7 @@
		from core.tests import test_utils
		import feconf

Fix part of #3905: Add lint checks for alphabetical import order #4343

Fix part of #3905: Add lint checks for alphabetical import order #4343

Conversation

apb7 commented Jan 3, 2018 • edited by DubeySandeep Loading

apb7 commented Jan 3, 2018

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

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 Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jan 3, 2018

apb7 commented Jan 3, 2018

codecov-io commented Jan 3, 2018 • edited Loading

Codecov Report

apb7 commented Jan 3, 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

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

apb7 commented Jan 6, 2018

nithusha21 commented Jan 6, 2018

apb7 commented Jan 6, 2018

seanlip left a comment

Choose a reason for hiding this comment

apb7 commented Jan 6, 2018

apb7 commented Jan 3, 2018 •

edited by DubeySandeep

Loading

seanlip Jan 4, 2018 •

edited

Loading

codecov-io commented Jan 3, 2018 •

edited

Loading

apb7 commented Jan 3, 2018 •

edited

Loading