Closed
Bug 1288432
Opened 8 years ago
Closed 7 years ago
[mozlint] Re-design the linter configuration mechanism
Categories
(Developer Infrastructure :: Lint and Formatting, defect)
Developer Infrastructure
Lint and Formatting
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ahal, Assigned: ahal)
References
Details
Attachments
(4 files)
Currently mozlint uses these .lint files which are actually python files that have a LINTER dictionary in them. Jgraham was already complaining about them, and I suspect more people will as well as they get more complex. It's probably a good idea to separate the config from the implementation. So I'm thinking the configs can be either yaml/json/ini files with an implementation value that points to a python import path. Something similar to: https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/legacy/kind.yml#5 I don't really care what type of config we use. I'm leaning towards yaml, but open to bikeshedding. Also, I'm not going to be actively working on this just yet. Just want to get it on file.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Hey bc, I know you aren't familiar with this code base, but I thought if you were interested in getting involved with linting doing some review might help. I'm happy to explain things, but if you'd prefer I find someone else to do this, please let me know. James, Mark, feel free to chime in on the first commit too if you're interested.
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8873993 [details] Bug 1288432 - Use new mozlint configuration for eslint linter, https://reviewboard.mozilla.org/r/145404/#review149372 ::: tools/lint/eslint/hook_helper.py:22 (Diff revision 1) > + # This is a hacky way to avoid both re-defining extensions and > + # depending on PyYaml in hg's python path. This will go away > + # once the vcs hooks are using mozlint directly (bug 1361972) > + with open(config_path) as fh: > + line = [l for l in fh.readlines() if 'extensions' in l][0] > + > + extensions = json.loads(line.split(':', 1)[1].replace('\'', '"')) > + return [e.lstrip('.') for e in extensions] I know this is pretty terrible.. We could alternatively duplicate the extensions list, but then they might get out of sync.
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8873993 [details] Bug 1288432 - Use new mozlint configuration for eslint linter, https://reviewboard.mozilla.org/r/145404/#review149692 Looks good.
Attachment #8873993 -
Flags: review?(standard8) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8873990 [details] Bug 1288432 - [mozlint] Use yaml for lint definitions and separate implementation of external linters, https://reviewboard.mozilla.org/r/145398/#review149738 Minor questions, but I must admit I'm only following along at this point. ::: python/mozlint/mozlint/cli.py:95 (Diff revision 1) > name = os.path.basename(f) > > - if not name.endswith('.lint.py'): > + if not name.endswith('.yml'): > continue > > - name = name.rsplit('.', 2)[0] > + name = name.rsplit('.', 1)[0] follows the existing pattern, but how about os.path.splitext(name)[0] instead ? ::: python/mozlint/mozlint/parser.py:60 (Diff revision 1) > - raise LinterParseError(path, "Invalid filename, linters must end with '.lint.py'!") > + raise LinterParseError(path, "Invalid filename, linters must end with '.yml'!") > + > + with open(path) as fh: > + config = yaml.load(fh) > + > + if not config: Is config guaranteed to be defined here? ::: python/mozlint/test/linters/explicit_path.yml:7 (Diff revision 1) > -# vim: set filetype=python: > - > -LINTER = { > - 'name': "ExplicitPathLinter", > - 'description': "Only lint a specific file name", > - 'rule': 'no-foobar', > + description: Only lint a specific file name > + rule: no-foobar > + include: > + - no_foobar.js > + type: string > + payload: foobar Will this work without the expected colon? ::: tools/lint/docs/create.rst:4 (Diff revision 1) > Adding a New Linter to the Tree > =============================== > > -A linter is a python file with a ``.lint`` extension and a global dict called LINTER. Depending on how > +A linter is a yaml file with a ``.yml`` extension. Depending on how the type of linter, there may redundant "on how" ::: tools/lint/docs/create.rst:4 (Diff revision 1) > Adding a New Linter to the Tree > =============================== > > -A linter is a python file with a ``.lint`` extension and a global dict called LINTER. Depending on how > +A linter is a yaml file with a ``.yml`` extension. Depending on how the type of linter, there may redundant "how"
Attachment #8873990 -
Flags: review?(bob) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8873991 [details] Bug 1288432 - Use new mozlint configuration for flake8 linter, https://reviewboard.mozilla.org/r/145400/#review149812 Seems ok, but I would like to understand why were are using a directory flake8_ with the trailing underscore. I'm missing why that is necessary. ::: taskcluster/docker/lint/Dockerfile:21 (Diff revision 1) > > # %include taskcluster/docker/recipes/install-mercurial.sh > ADD topsrcdir/taskcluster/docker/recipes/install-mercurial.sh /build/install-mercurial.sh > ADD system-setup.sh /tmp/system-setup.sh > -# %include tools/lint/flake8/flake8_requirements.txt > -ADD topsrcdir/tools/lint/flake8/flake8_requirements.txt /tmp/flake8_requirements.txt > +# %include tools/lint/flake8_/flake8_requirements.txt > +ADD topsrcdir/tools/lint/flake8_/flake8_requirements.txt /tmp/flake8_requirements.txt Why are we using flake8_ here?
Attachment #8873991 -
Flags: review?(bob) → review+
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8873991 [details] Bug 1288432 - Use new mozlint configuration for flake8 linter, https://reviewboard.mozilla.org/r/145400/#review149812 > Why are we using flake8_ here? It's because these dirs need to get added so sys.path for the import to work, but then when it was called 'flake8' it started interfering with the real flake8 package. We could come up with another naming scheme if you prefer.. It just can't be 'flake8'.
Comment 11•7 years ago
|
||
Ah, ok. That explains it. I'm not really that opposed to flake8_ but it isn't obvious on the face of it why. Perhaps flake8_lint ? We could use the same pattern for other linters that have similar problems in the future. I'll leave it up to you.
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8873992 [details] Bug 1288432 - Use new mozlint configuration for wpt and wpt_manifest linters, https://reviewboard.mozilla.org/r/145402/#review150662
Attachment #8873992 -
Flags: review?(james) → review+
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8873990 [details] Bug 1288432 - [mozlint] Use yaml for lint definitions and separate implementation of external linters, https://reviewboard.mozilla.org/r/145398/#review149738 Thanks for the review! > Is config guaranteed to be defined here? I think so, if there was a problem opening or loading the yaml then there would be an exception raised and we'd never get to this line. > Will this work without the expected colon? Yep, assuming you're talking about the include directive, that's how a list is defined in yaml.
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Bob Clary [:bc:] from comment #11) > Ah, ok. That explains it. I'm not really that opposed to flake8_ but it > isn't obvious on the face of it why. Perhaps flake8_lint ? We could use the > same pattern for other linters that have similar problems in the future. > I'll leave it up to you. Thanks, I think I'm just going to leave it for now. It's only a problem we'd run into with python linters, and I don't think we'll be adding any more of those anytime soon. I was also thinking of creating a 'linters' directory and putting everything under there.. But at this point I'd prefer to do that in a follow-up bug.
Comment 15•7 years ago
|
||
Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a9e96c220249 [mozlint] Use yaml for lint definitions and separate implementation of external linters, r=bc https://hg.mozilla.org/integration/autoland/rev/97d503af98d9 Use new mozlint configuration for flake8 linter, r=bc https://hg.mozilla.org/integration/autoland/rev/0eb21ab64fcd Use new mozlint configuration for wpt and wpt_manifest linters, r=jgraham https://hg.mozilla.org/integration/autoland/rev/172f1077e83f Use new mozlint configuration for eslint linter, r=standard8
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a9e96c220249 https://hg.mozilla.org/mozilla-central/rev/97d503af98d9 https://hg.mozilla.org/mozilla-central/rev/0eb21ab64fcd https://hg.mozilla.org/mozilla-central/rev/172f1077e83f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Product: Testing → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•