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

String modes #22

Merged
merged 3 commits into from
Jul 6, 2015
Merged

String modes #22

merged 3 commits into from
Jul 6, 2015

Conversation

miki725
Copy link
Contributor

@miki725 miki725 commented May 29, 2015

flake8 can be fooled. for example:

class Foo(object): pass


+
+
class Bar(object):pass

technically there is now a violation on Bar since there are too many blank lines however flake8-diff will not detect it since the diff only added the blank lines which is not where violation is.

The idea of strict modes is to allow to choose how files are evaluated.

  • only_lines - default and existing behaviour which only counts violations when that specific line is added and it has a violation
  • file - uses all violations from any modified files in the diff

possible strict modes for the future

  • percent_file - uses all violations from any modified files when specific percent of lines was modified in the file. for example if only 2 lines were modified in 1000 line file, dont bother, but on the other hand if 600 lines were modified, then probably a good idea to lint the whole file, same as file strict mode

@miki725
Copy link
Contributor Author

miki725 commented May 30, 2015

cc @agamdua

@gregarmer gregarmer self-assigned this Jun 15, 2015
elif strict_mode == 'file':
return True
else:
msg = 'Strict mode {} is not supportec'.format(strict_mode)
Copy link
Collaborator

Choose a reason for hiding this comment

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

supported

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably handle this check much earlier on and expect to get a valid option by this point. Suggestion would be a check in main.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its already checked in main.py. This is just in case..

sorry for the typo. will fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's already checked in main.py, then there's absolutely no possible way this else condition could ever be executed accidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only possibility is if someone is using the Python API directly. not sure if that is likely. remove else?

@miki725
Copy link
Contributor Author

miki725 commented Jun 15, 2015

@gregarmer I think I addressed both comment in miki725@42a9e31. Let me know what you think.

  • It removes else since it is not necessary as AttributeError will be thrown in __init__ if strict mode is not implemented.
  • file strict mode is cached by replacing the whole function which always returns True

@@ -167,7 +200,8 @@ def process(self):
matches = FLAKE8_LINE.match(violation)
if matches:
violation_details = matches.groupdict()
if violation_details['line_number'] in violated_lines:
if self.should_include_violation(violation_details,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also be renamed to _should_include_violation()

@gregarmer
Copy link
Collaborator

@miki725 there's just one method rename left, then we can merge this.

@miki725
Copy link
Contributor Author

miki725 commented Jul 6, 2015

thanks @gregarmer for reminder. just updated

gregarmer added a commit that referenced this pull request Jul 6, 2015
@gregarmer gregarmer merged commit 28b4f82 into dealertrack:master Jul 6, 2015
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