Skip to content

[Koa Backport] Fix linting as it relates to import_shims, and vice versa - #25678

Merged
kdmccormick merged 3 commits into
open-release/koa.masterfrom
kdmccormick/import-shims-pylint-koa
Nov 30, 2020
Merged

[Koa Backport] Fix linting as it relates to import_shims, and vice versa#25678
kdmccormick merged 3 commits into
open-release/koa.masterfrom
kdmccormick/import-shims-pylint-koa

Conversation

@kdmccormick

@kdmccormick kdmccormick commented Nov 24, 2020

Copy link
Copy Markdown
Member

@regisb @nedbat

Context

This is a port of https://github.com/edx/edx-platform/pull/25632 onto open-release/koa.master.

I believe it should be backported because pylint usage is currently partially broken on koa.master. For example:

$ pylint manage.py
Traceback (most recent call last):
  File "/home/regis/venvs/openedx3.8/bin/pylint", line 8, in <module>
    sys.exit(run_pylint())
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/__init__.py", line 23, in run_pylint
    PylintRun(sys.argv[1:])
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/lint.py", line 1689, in __init__
    linter.load_config_file()
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/config.py", line 736, in load_config_file
    self.global_set_option(option, value)
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/config.py", line 629, in global_set_option
    self._all_options[opt].set_option(opt, value)
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/lint.py", line 729, in set_option
    checkers.BaseTokenChecker.set_option(self, optname, value, action, optdict)
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/config.py", line 825, in set_option
    value = _validate(value, optdict, optname)
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/config.py", line 234, in _validate
    return _call_validator(_type, optdict, name, value)
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/config.py", line 215, in _call_validator
    return VALIDATORS[opttype](optdict, option, value)
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/config.py", line 175, in _regexp_csv_validator
    return [_regexp_validator(_, name, val) for val in _csv_validator(_, name, value)]
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/config.py", line 175, in <listcomp>
    return [_regexp_validator(_, name, val) for val in _csv_validator(_, name, value)]
  File "/home/regis/venvs/openedx3.8/lib/python3.8/site-packages/pylint/config.py", line 170, in _regexp_validator
    return re.compile(value)
  File "/home/regis/install/Python-3.8.5/lib/python3.8/re.py", line 252, in compile
    return _compile(pattern, flags)
  File "/home/regis/install/Python-3.8.5/lib/python3.8/re.py", line 304, in _compile
    p = sre_compile.compile(pattern, flags)
  File "/home/regis/install/Python-3.8.5/lib/python3.8/sre_compile.py", line 764, in compile
    p = sre_parse.parse(p, flags)
  File "/home/regis/install/Python-3.8.5/lib/python3.8/sre_parse.py", line 948, in parse
    p = _parse_sub(source, state, flags & SRE_FLAG_VERBOSE, 0)
  File "/home/regis/install/Python-3.8.5/lib/python3.8/sre_parse.py", line 443, in _parse_sub
    itemsappend(_parse(source, state, verbose, nested + 1,
  File "/home/regis/install/Python-3.8.5/lib/python3.8/sre_parse.py", line 668, in _parse
    raise source.error("nothing to repeat",
re.error: nothing to repeat at position 0

The problematic line is ignore-patterns = **/import_shims/**/*.py in pylintrc, which was trying to exclude the import_shims/ directory tree from linting. This failure was not showing up in CI because that regex only breaks under certain conditions.

For more context on what the heck import_shims is in the first place, see my forum post about edx-platform import changes.

This PR

Instead of trying to fix the regex, I decided it'd be better to actually fix all the linting violations within import_shims/ so that we don't have to exclude the directory tree at all. Furthermore, this PR removes import_shims/lms and import_shims/studio from the sys.path in pylintrc, meaning that pylint will complain if it encounters any deprecated edx-platform import paths.

Notes for reviewers

  • I've broken this PR's changes into three commits. The first two (Fix import_shims pylint..., and Do not shim...) deserve actual review. The third one (Regenerate import_shims) is just the result of a script, and modifies hundreds of stub files in the same exact way. I recommend just glancing at that commit.

  • The first two commits are identical to what I committed to master. For the third commit, I specifically re-generated the import shims for the set of modules that exist in Koa, which is slightly different than the set of modules that exist on master currently. Thus, this is not a direct cherry-pick of the original change.

  • When merging, I recommend squash+merge, as the individual commits do not stand on their own as logical changes.

  • I have not run the validation suite on this change locally; I figured CI would take care of that. It looks like CI isn't running on my change, though, so let me know if I should do it myself.

The import_shims/ directory had thousands of pylint
violations, but they didn't matter because we ignored
the directory during linting.

Now, that directory ignoring configuration seems to be
problematic when running pylint on individual files locally.

Also, it seems prudent to lint import_shims/ anyway,
because there is still production code that relies on it.
In other words, when running pylint, do not
enable the import shims. So, the old-style
imports (`from student import models`) are
invalid in the eye of pylint now.

We do this because:
* New sys.path-hacking in pylintrc seems to break the
  diff-quality tooling.
* edx-platform should have no old-style imports,
  so pylint doesn't need to know about the import shims.
* We actually *want* quality failures to happen if
  anyone reintroduces old-style imports to edx-platform.
Command:
$ import_shims/regenerate_current_shims.sh
@kdmccormick
kdmccormick marked this pull request as ready for review November 24, 2020 17:39
@kdmccormick

kdmccormick commented Nov 24, 2020

Copy link
Copy Markdown
Member Author

@nedbat @regisb -- This should be ready for review. However, we're seeing some production issues on edX.org since it was merged and deployed. I don't think the issues are related, but I would hold off on merging this into Koa just in case

@kdmccormick

Copy link
Copy Markdown
Member Author

Production issues have been resolved; they weren't related to this change.

@nedbat

nedbat commented Nov 30, 2020

Copy link
Copy Markdown
Contributor

I've installed this code, and the LMS comes up, so let's merge it for more thorough community testing.

@kdmccormick
kdmccormick merged commit b0bc2c2 into open-release/koa.master Nov 30, 2020
@kdmccormick
kdmccormick deleted the kdmccormick/import-shims-pylint-koa branch November 30, 2020 22:41
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Apr 14, 2021
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request May 24, 2021
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Jun 9, 2021
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Jun 9, 2021
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Aug 9, 2021
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Aug 11, 2021
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Jan 6, 2022
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Jan 6, 2022
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Jan 7, 2022
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Jan 7, 2022
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Feb 11, 2022
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
sambapete pushed a commit to EDUlib/edx-platform that referenced this pull request Feb 11, 2022
…rsa (openedx#25678)

Should resolve "re.error: nothing to repeat at position 0"
when running pylint locally.

Changes:
* Fix import_shims pylint violations
* Include import_shims directory in linting
* Stop shimming imports when running pylint
* Regenerate import_shims (for koa.master)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants