Closed Bug 1238587 Opened 10 years ago Closed 10 years ago

Improve and maintain code quality with linting tools

Categories

(developer.mozilla.org Graveyard :: BrowserCompat, defect)

Product:
developer.mozilla.org Graveyard
Component:
BrowserCompat
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jwhitlock, Unassigned, Mentored)

Details

(Keywords: in-triage, Whiteboard: [bc:infra][bc:milestone=bicycle])

What problem would this feature solve? ====================================== The BrowserCompat code quality is maintained by listing tools that check for formatting and quality issues with each check-in, as well as code reviews for significant chunks of work. As time passes, new "linting" [1] tools are developed, and developers argue for new quality standards. Python has enforced code quality in its DNA, controversially choosing to use significant whitespace for code blocks [2], and has only gotten more judgmental with PEP-8 [3] and PEP-257 [4]. Internally consistent code is easier to read, code reviews can focus on the content instead of the formatting, and it becomes easier to see logic errors. These higher standards require updating the code, and our policies require linking changes to Bugzilla. [1] https://en.wikipedia.org/wiki/Lint_(software) [2] http://www.c2.com/cgi/wiki?PythonWhiteSpaceDiscussion [3] https://www.python.org/dev/peps/pep-0008/ [4] https://www.python.org/dev/peps/pep-0257/ Who has this problem? ===================== Core contributors to MDN How do you know that the users identified above have this problem? ================================================================== The flake8-docstrings [5] plugin in used in BC, but several checks are disabled [6], and could be re-enabled. When willkg joined the project and started code reviews, he suggested new standards for assertions [7], and quotes [8]. Similar projects have been proposed for MDN/Kuma [9]. [5] https://gitlab.com/pycqa/flake8-docstrings [6] https://github.com/mdn/browsercompat/blob/master/setup.cfg#L32-L44 [7] https://github.com/mdn/browsercompat/pull/89#discussion_r49111291 [8] https://github.com/mdn/browsercompat/pull/89#discussion_r49111531 [9] https://bugzilla.mozilla.org/show_bug.cgi?id=1237977 How are the users identified above solving this problem now? ============================================================ For small changes, PRs are amended to make the change. For large changes, the linting tools are reconfigured or augmented, and usually included in the PRs for different "feature" bugs in Bugzilla. Do you have any suggestions for solving the problem? Please explain in detail. ============================================================================== Leave this open as a tracking bug for ideas about code quality, and possible work for those who want to familiarize themselves with the code base. Is there anything else we should know? ====================================== Some initial work that could appear under this bug: * Add and enable https://pypi.python.org/pypi/flake8-import-order to enforce import declaration order * Add and enable https://pypi.python.org/pypi/flake8-quotes to cleanup single- and double-quote usage * Re-enable checks for the PEP 257 docstring checker
Severity: enhancement → normal
Keywords: in-triage
OS: Other → All
Whiteboard: [specification][type:feature] → [bc:infra]
Mentor: jwhitlock
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/a992b22aacff27066f9528d0983693b30c943dd4 bug 1238587 - Bump easy requirements requirements.txt: * amqp 1.4.7 -> 1.4.9 - unrelated fixes * billiard - 3.3.0.21 -> 3.3.0.22 - unrelated fixes * Django 1.8.6 -> 1.8.8 - unrelated security updates * django-extensions 1.5.9 -> 1.6.1 - Django 1.9 compat * django-filter 0.11.0 -> 0.12.0 - Django 1.8/1.9 compat, ordering fix * django-mptt 0.7.4 -> 0.8.0 - Django 1.9 compat, unrelated fixes * django-nose 1.4.2 -> 1.4.3 - Django 1.9 compat * django-pylibmc 0.6.0 -> 0.6.1 - Django 1.9 compat * django-simple-history 1.6.3 -> 1.7.0 - Django 1.9 compat, admin * updates * django-sortedm2m 1.0.2 -> 1.1.1 - Django 1.9 compat * djangorestframework 3.3.1 -> 3.3.2 - Django 1.8/1.9 compat * gunicorn 19.3.0 -> 19.4.5 - Big release, Py3 support, ASCII-only * headers * kombu 3.0.29 -> 3.0.33 - Redis transport fixes * Markdown 2.6.4 -> 2.6.5 - unrelated fix * newrelic 2.58.0.43 -> 2.60.0.46 - unrelated hotfixes, new event * feature * requests 2.8.1 -> 2.9.1 - unrelated fixes * requests-oauthlib 0.5.0 -> 0.6.0 - protocol fixes requirements-dev.txt: * alabaster 0.7.6 -> 0.7.7 - style fixes * Babel 2.1.1 -> 2.2.0 - unrelated features * check-manifest 0.28 -> 0.30 - unrelated fixes * coverage 4.0.2 -> 4.0.3 - unrelated fixes * flake8 2.5.0 -> 2.5.1 - unrelated fixes * ipython 4.0.0 -> 4.0.3 - unrelated fixes * pep8 1.6.2 -> 1.7.0 - unrelated fixes * py 1.4.30 -> 1.4.31 - unrelated fixes * Pygments 2.0.2 -> 2.1 - unrelated lexers / styles * snowballstemmer 1.2.0 -> 1.2.1 - unrelated fixes * Sphinx 1.3.1 -> 1.3.4 - many fixes * tox 2.2.1 -> 2.3.1 - environment setup fixes * virtualenv 13.1.2 -> 14.0.0 - requirements bump * Werkzeug 0.11.2 -> 0.11.3 - unrelated fixes https://github.com/mdn/browsercompat/commit/88c4689fffca0188db17bf0c939461dab8a263b0 bug 1238587 - Add docstrings for public modules Enable PEP257 warning D100 (Missing docstring in public module). For data migrations, add an explanatory docstring. For schema migrations, add "# flake8: noqa" comment (don't try to fix auto-generated code). https://github.com/mdn/browsercompat/commit/83c1194c59a95f070add40d712b78add4fcd7341 bug 1238587 - Fix error message in migration Change copy-pasted error message to reflect the actual issue. https://github.com/mdn/browsercompat/commit/68550c7e7ed52c9c1e313ba8bfc7ef8baf04fb1f bug 1238587 - Fix whitespace for class docstrings Remove ignored PEP257 warnings: * D204 - 1 blank line required after class docstring * D211 - No blank lines allowed before class docstring https://github.com/mdn/browsercompat/commit/6e70aaaaed3efa3b8bff3e682567fb4e15e5f30a bug 1238587 - Add period for first docstring line Enables D400 - First line should end with a period https://github.com/mdn/browsercompat/commit/cbd1aba7d6a09308a01d00fcedc2b38175b28080 bug 1238587 - Add docstrings for packages Enables D104 - Missing docstring in public package Also, update the ignored error counts https://github.com/mdn/browsercompat/commit/c8a72099324af9a89a7df90342121a241b0ddfbb bug 1238587 - Ignore docstrings in tests Docstrings have not been used for tests because they make nose output less useful. When a test fails, it is useful to know the filename, test class, and test method, so that the test can be run in isolation. So, most tests have verbose names, and occasionally comments describing the test purpose. This adds a nose plugin to ignore test docstrings, so that they can be used without the negative side effects. This may mean that PEP 257 warning D102 (missing docstring in public methods) could be re-enabled in the future. https://github.com/mdn/browsercompat/commit/26abb56b0bb4a7d3ebe82383a2c948c3b577e2e5 bug 1238587 - Change double quotes to single Add flake8-quotes linter, and fix instances of unneeded double quotes identified by the tool. https://github.com/mdn/browsercompat/commit/4a431ad635d830dad9c3f0e3796c897c08d87144 bug 1238587 - Update ignored issue counts https://github.com/mdn/browsercompat/commit/34679e1305a25ebafa8530cb5e30c12f5833aa2c bug 1238587 - Add TravisCI IRC notifications https://github.com/mdn/browsercompat/commit/9a776acb36716484c2bbe501b84efab9384f82fe Merge pull request #90 from mdn/more_flake8_1238587 bug 1238587 - Quotes, comments, periods, and whitespace r+ willkg
Whiteboard: [bc:infra] → [bc:infra][bc:milestone=bicycle]
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/f4ca370d3e88da73f7a421419ebc730042fd23ef bug 1238587 - Run TravisCI under Python 3 Update integration tests to use Python2/3 compatibile code, instead of Python 2-only code. https://github.com/mdn/browsercompat/commit/0851bf9404e37f9ba406a40b474a5b709a076f4d bug 1238587 - Don't mutate dict during iteration Python 3.5 has reimplemented OrderedDict, and does not allow mutating it during iteration. Reimplement loop to take note of changes to make, and make the changes after iteration. https://github.com/mdn/browsercompat/commit/3c37c821af31df6ff794c9cd0597e0f576854637 bug 1238587 - Preserve order on JSON parsing Parse JSON into an OrderedDict to preserve declaration order and to make the v1 JSON parser tests more reliably hit all branches. https://github.com/mdn/browsercompat/commit/8feaf48905cee1e895bc22acaacfa087d943e09f bug 1238587 - Switch Python 3.4 to 3.5 https://github.com/mdn/browsercompat/commit/f765415314216eb2bd64e136f34407165e0ab6bb bug 1238587 - Move requirements files to subfolder In preparation to split requirements files, move to subfolder. Leave a stub requirements.txt in the root folder for Heroku https://github.com/mdn/browsercompat/commit/055e0505f0cb3b9067d2d7d9dcbb8999b96b07a9 bug 1238587 - Split requirement files Split requirements into several files, to minimize the packages installed in tox. Developers should install requirements/development.txt. https://github.com/mdn/browsercompat/commit/d9741f13029283b482b8ead6d29b84d670bf91e0 bug 1238587 - Add constraints.txt Constraints tell pip "if you install this package, use this version". This simplifies the other requirements files, doesn't install dependencies when not needed, but still makes installs repeatable. New in pip 7. https://github.com/mdn/browsercompat/commit/ee30f1a270e8d34b1cfae4de980ee57f212669ae bug 1238587 - Add hashes pep 8 will verify that all installed packages are in a requirement file (no implicit dependencies) and that the package hash matches. https://github.com/mdn/browsercompat/commit/62f162e78efe22165e103553b586ddf8fecd55ff bug 1238587 - Add more package hashes Use hashin to download and calculate hashes for alternate package files https://github.com/mdn/browsercompat/commit/15e850f01775c6cafcd41b55d1e1f128d4d23e66 bug 1238587 - Add requirements for readthedocs.org RTD breaks when installing packages against C libraries, so it doesn't use any of the regular requirements files. requirements-rtd.txt can be removed when the RTD configuration has been updated. https://github.com/mdn/browsercompat/commit/b406e854997c53fdbce0b1b3ad61506a196218a3 Merge pull request #95 from mdn/pip8_1238587 bug 1238587 - Python 3.4 -> 3.5, pip constraints and hashes r~=willkg
Commit pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/2d6310a098f046cc6a70dddd215edb0cdb16c86f bug 1238587 - Change bytes in initial migration Local tests under Python 3.5 are having trouble with migration 0010 because the original default for this JSONField was the byte sequence b'[]', and it needs to be the unicode string '[]'. That's the effect of the migration, but it has to run under Python 2.7, which is more flexible about what is valid JSON. So, hacking it by going back in time and fixing the intial migration. Resetting migrations will probably be part of the work on bug 1230306 (Convert to UUIDs) and bug 1153329 (renaming project to match browsercompat), so this hack will soon be hidden in revision history.
Are we done with this or do we have still pending linting errors to fixe?
Flags: needinfo?(jwhitlock)
I meant this to be a "evergreen" bug, like bug 957802, "streamline new contributor setup", to cover issues brought up during code reviews. From bug 1237977, there are a few more we could use: https://pypi.python.org/pypi/pep8-naming https://pypi.python.org/pypi/flake8-blind-except https://pypi.python.org/pypi/flake8-commas https://pypi.python.org/pypi/flake8-debugger https://pypi.python.org/pypi/flake8-import-order or https://pypi.python.org/pypi/flake8-isort https://pypi.python.org/pypi/flake8-mock https://pypi.python.org/pypi/flake8-print https://pypi.python.org/pypi/flake8_tuple If we need to close this bug, then I can pick up this work, or we could prefix it with "Q1 2016" and close it on March 21.
Flags: needinfo?(jwhitlock)
Mmmh, I don't really like bugs that stay open forever. It doesn't really help keeping track of the necessarily work to be done and can be somewhat discouraging. However I understand the on going needs for quality code check. So, I suggest the following for this bug: * For any new code landing in the github repo (starting today) it is forbidden to merge a PR if it doesn't pass the linting requirements. All associated bugs to a given PR should not be marked as resolved as long as the linting check isn't green. Can we have Travis enforcing this? (If no, please open a follow up bug to do this as it is pointless to do linting if we do not enforce rules check). * For legacy code that doesn't pass the linting let's keep that bug open until we are ready to deliver the Bicycle milestone. When we are ready to deliver, let's recheck that bug to: 1. close it, 2. Open a follow up bug for the remaining work, 3. let's do this at the end of each milestone util we are all green :).
TravisCI already checks this, it's the "TOXENV=flake8" check: https://travis-ci.org/mdn/browsercompat Closing as fixed for now, will open new bugs as needed for new linting requirements.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Commit pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/5abb4bd8da568d3e42f3e47163af0be0f9403d9e bug 1238587 - Remove requirements-rtd.txt ReadTheDocs is now configured to look at requirements/readthedocs.txt
Product: developer.mozilla.org → developer.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.