Closed Bug 1288432 Opened 3 years ago Closed 2 years ago

[mozlint] Re-design the linter configuration mechanism

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

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.
Depends on: 1358540
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Blocks: 1369792
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.
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 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 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 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+
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'.
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 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+
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.
(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.
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
Depends on: 1371597
Depends on: 1375166
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.