Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove match args to allow pydocstyle defaults #243

Merged

Conversation

Arnatious
Copy link
Contributor

Changes --match to use pydocstyle default

  • Current: .*\.py
  • New (Default): (?!test_).*\.py
    Result: don't check 'test_*' files for documentation

Changes --match-dir to use pydocstyle default

  • Current: [^\._].*
  • New (Default): [^\.].*
    Result: include private ('_*') package directories in checks

Signed-off-by: Ted Kern ted.kern@canonical.com

@Arnatious Arnatious changed the title remove match args to allow pydocstyle defaults Remove match args to allow pydocstyle defaults May 15, 2020
Copy link

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in support of using the default behavior, although admittedly this may have wide-ranging ramifications if non-public modules were never linted. I think this is technically correct, but I also don't like the idea of code suddenly starting to fail the linter without the code changing. We might consider exposing this as an option instead. Curious to get other thoughts.

Signed-off-by: Ted Kern <ted.kern@canonical.com>
@dirk-thomas dirk-thomas force-pushed the arnatious/pep257_check_private_modules branch from 1fa3d4d to 2cd12a5 Compare June 13, 2020 03:17
@dirk-thomas
Copy link
Contributor

dirk-thomas commented Jun 13, 2020

I rebased your branch since the target branch has changed quite a bit since the fork.

A first CI build on Linux to see how the modified config affect the result for the existing code: Build Status

@dirk-thomas
Copy link
Contributor

I also don't like the idea of code suddenly starting to fail the linter without the code changing.

This is certainly not something I would propose to be backported. But changing the default in the rolling distro is imo acceptable. Especially since the chances that it affects existing code is rather low (considering that there is not a single new warning due to this change in the whole ros2.repos file).

So I am planning to merge this within a day if there are no strong opinions not to accept the patch as is.

@kyrofa
Copy link

kyrofa commented Jun 15, 2020

So I am planning to merge this within a day if there are no strong opinions not to accept the patch as is.

Not from me, you convinced me with:

there is not a single new warning due to this change in the whole ros2.repos file

@dirk-thomas dirk-thomas merged commit 404c8fa into ament:master Jun 16, 2020
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.

3 participants