Closed Bug 1406650 Opened 7 years ago Closed 7 years ago

Make build/*.py flake8 compatible and add them to the list of files to check

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement)

enhancement
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(1 file)

      No description provided.
Comment on attachment 8916260 [details]
Bug 1406650 - Make build/*.py and a few other files flake8 compatible and add them to the list of files to check

https://reviewboard.mozilla.org/r/187484/#review193580

This looks good to me, but a build peer will need to grant final review here. Try mshal or chmanchester.

::: commit-message-6b049:1
(Diff revision 2)
> +hBug 1406650 - Make build/*.py and a few other files flake8 compatible and add them to the list of files to check r?ahal

extra 'h'. This might fail the server side commit hook.

::: tools/lint/flake8.yml:6
(Diff revision 2)
>  ---
>  flake8:
>      description: Python linter
>      include:
> +        - configure.py
> +        - build/*.py

This will only lint files that are direct children of `/build`. Was that intentional? If you wanted to lint all files under there, this should be changed to just `build`. I suspect doing this will result in a lot more failures.
Attachment #8916260 - Flags: review?(ahalberstadt)
Comment on attachment 8916260 [details]
Bug 1406650 - Make build/*.py and a few other files flake8 compatible and add them to the list of files to check

https://reviewboard.mozilla.org/r/187484/#review193580

thanks

> extra 'h'. This might fail the server side commit hook.

Oups, thanks

> This will only lint files that are direct children of `/build`. Was that intentional? If you wanted to lint all files under there, this should be changed to just `build`. I suspect doing this will result in a lot more failures.

Yes, this is the goal for now.
There are subdirectories not consistent to pep8
Comment on attachment 8916260 [details]
Bug 1406650 - Make build/*.py and a few other files flake8 compatible and add them to the list of files to check

Thanks for the clarification.
Attachment #8916260 - Flags: review?(ahalberstadt) → review+
Assignee: nobody → sledru
Comment on attachment 8916260 [details]
Bug 1406650 - Make build/*.py and a few other files flake8 compatible and add them to the list of files to check

https://reviewboard.mozilla.org/r/187484/#review193774

::: build/appini_header.py:30
(Diff revision 3)
> -    appdata = dict(("%s:%s" % (s, o), config.get(s, o)) for s in config.sections() for o in config.options(s))
> +        pass
> +    appdata = dict(("%s:%s" % (s, o), config.get(s, o))
> +                   for s in config.sections() for o in config.options(s))
>      appdata['flags'] = ' | '.join(flags) if flags else '0'
> -    appdata['App:profile'] = '"%s"' % appdata['App:profile'] if 'App:profile' in appdata else 'NULL'
> +    appdata['App:profile'] = ('"%s"' %
> +                              appdata['App:profile'] if 'App:profile' in appdata else 'NULL')

Can we break this line before the `if` instead?

::: build/gen_test_packages_manifest.py:47
(Diff revision 3)
> -                        help="Name of the \"common\" archive, a package to be used by all harnesses.")
> +                        help="Name of the \"common\" archive, a package to be used by all "
> +                        "harnesses.")

Is flake8 ok with "harnesses" directly below "Name" in this and other cases like it? (At least as mozreview is showing this to me, "harnesses" is directly below `help`).

::: build/upload.py:219
(Diff revision 3)
> -        properties = {prop: 'UNKNOWN' for prop, condition in property_conditions}
> +        properties = {prop: 'UNKNOWN' for prop,
> +                      condition in property_conditions}

Can we break this line before `in` instead?
Attachment #8916260 - Flags: review?(cmanchester) → review+
Comment on attachment 8916260 [details]
Bug 1406650 - Make build/*.py and a few other files flake8 compatible and add them to the list of files to check

https://reviewboard.mozilla.org/r/187484/#review193774

> Is flake8 ok with "harnesses" directly below "Name" in this and other cases like it? (At least as mozreview is showing this to me, "harnesses" is directly below `help`).

yeah, flake8 is happy with that
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 48dcf372b45a -d 4b4228043dd8: rebasing 426005:48dcf372b45a "Bug 1406650 - Make build/*.py and a few other files flake8 compatible and add them to the list of files to check r=chmanchester" (tip)
merging tools/lint/flake8.yml
warning: conflicts while merging tools/lint/flake8.yml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 48dcf372b45a -d 44d7a43c481a: rebasing 426049:48dcf372b45a "Bug 1406650 - Make build/*.py and a few other files flake8 compatible and add them to the list of files to check r=chmanchester" (tip)
merging build/build-clang/build-clang.py
merging tools/lint/flake8.yml
warning: conflicts while merging tools/lint/flake8.yml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6074db12d685
Make build/*.py and a few other files flake8 compatible and add them to the list of files to check r=chmanchester
Attachment #8916260 - Flags: review?(ahalberstadt)
https://hg.mozilla.org/mozilla-central/rev/6074db12d685
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.