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

Allow skipping decorated functions #204

Merged
merged 2 commits into from Oct 16, 2016

Conversation

Projects
2 participants
@larsoner
Copy link
Contributor

larsoner commented Sep 6, 2016

@Nurdok I think you talked about adding an option like this, see if it's what you have in mind. A user can now do e.g.:

--ignore-decorators=wraps

Or in my case, I will add this to my config:

ignore-decorators = copy_.*_doc_to_.*
@larsoner

This comment has been minimized.

Copy link
Contributor

larsoner commented Sep 6, 2016

If this seems to work, I'm happy to tackle #171 next, since I'll want that, too.

@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Oct 13, 2016

Please merge from default, this PR is out of date with the recent refactoring.

@larsoner larsoner force-pushed the larsoner:skip-dec branch 2 times, most recently from 4594f1d to ed19ba2 Oct 13, 2016

@larsoner

This comment has been minimized.

Copy link
Contributor

larsoner commented Oct 14, 2016

@Nurdok merge wasn't possible because of how bad the split was, but I started fresh and manually copied the changes into the necessary places. Tests pass locallay and Travis is happy.

@@ -10,7 +10,8 @@
from . import violations
from .config import IllegalConfiguration
# TODO: handle
from .parser import *
from .parser import (Package, Module, Class, NestedClass, Definition, AllError,
Method, Function, NestedFunction, Parser, StringIO)

This comment has been minimized.

@larsoner

larsoner Oct 14, 2016

Contributor

This one wasn't strictly necessary (orthogonal to this PR, maybe violates style prefs?) but it made it a lot easier to make changes because the automated pyflakes checking works much better without any * imports

This comment has been minimized.

@Nurdok

Nurdok Oct 14, 2016

Member

That's great actually - you can remove the "TODO" comment as this is what it meant, forgot to address it when I was refactoring.

@larsoner larsoner force-pushed the larsoner:skip-dec branch from ed19ba2 to 102a1eb Oct 14, 2016

if definition.skipped_error_codes != 'all' and \
not any(ignore_decorators is not None and
len(ignore_decorators.findall(dec.name))
for dec in definition.decorators):

This comment has been minimized.

@Nurdok

Nurdok Oct 14, 2016

Member

This if is too long. Break it up by assigning to local variables, something like:

checking_for_errors = ..
skipping_decorator = ..
if checking_for_errors and not skipping_decorator:
    # run the check

Think about better names, though :)

@@ -139,31 +141,42 @@ def _get_matches(config):
match_dir_func = re(config.match_dir + '$').match
return match_func, match_dir_func

def _get_ignore_dec(config):

This comment has been minimized.

@Nurdok

Nurdok Oct 14, 2016

Member

I prefer if you use the full word decorator, as this is not a common shorthand (like func).

match_dir=match_dir)
kwargs = dict(checked_codes=error_codes)
for key in ('match', 'match_dir', 'ignore_decorators'):
kwargs[key] = getattr(child_options, key) \

This comment has been minimized.

@Nurdok

Nurdok Oct 14, 2016

Member

You can do:

kwargs[key] = getattr(child_options, key) or getattr(parent_config, key)
kwargs = dict(checked_codes=checked_codes)
for key in ('match', 'match_dir', 'ignore_decorators'):
kwargs[key] = getattr(cls, 'DEFAULT_{0}_RE'.format(key.upper())) \
if getattr(options, key) is None and use_dafaults \

This comment has been minimized.

@Nurdok

Nurdok Oct 14, 2016

Member

use_dafaults -> use_defaults

This comment has been minimized.

@larsoner

larsoner Oct 16, 2016

Contributor

I figured that was done intentionally previously for some reason. Happy to change it though.

@@ -518,12 +525,20 @@ def _create_option_parser(cls):
"matches all dirs that don't start with "
"a dot").format(cls.DEFAULT_MATCH_DIR_RE))

# Decorators
option('--ignore-decorators', metavar='<decorators>', default=None,

This comment has been minimized.

@Nurdok

Nurdok Oct 14, 2016

Member

You also need to update the docs in both the CLI section and the release notes.

@@ -518,12 +525,20 @@ def _create_option_parser(cls):
"matches all dirs that don't start with "
"a dot").format(cls.DEFAULT_MATCH_DIR_RE))

# Decorators
option('--ignore-decorators', metavar='<decorators>', default=None,

This comment has been minimized.

@Nurdok

Nurdok Oct 14, 2016

Member

I wonder, should there be special handling of "qualified" decorators? e.g., @wraps vs @functools.wraps.

This comment has been minimized.

@larsoner

larsoner Oct 16, 2016

Contributor

That can be dealt with via regex, right?

@larsoner larsoner force-pushed the larsoner:skip-dec branch from 102a1eb to daeda05 Oct 16, 2016

@larsoner

This comment has been minimized.

Copy link
Contributor

larsoner commented Oct 16, 2016

Thanks for the review @Nurdok. Rebased after conflicts and then addressed your comments.

@Nurdok

Nurdok approved these changes Oct 16, 2016

@Nurdok Nurdok merged commit ccfafc9 into PyCQA:master Oct 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Oct 16, 2016

Merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment