codingjoe / relint

Write your own linting rules using regular expressions.
MIT License
57 stars 12 forks source link

Does not act on all files? (Despite lack of "filename: ..." and "exclude: ...") #4

Closed moseb closed 5 years ago

moseb commented 5 years ago

I have a weird problem here that maybe you have an idea about. I'm tryint to find uses of mock where it should be unittest.mock for Python, instead. So I have:

$ cat .relint.yml 
- name: "PyPI mock instead of Python 3 unittest.mock"
  pattern: "^(import mock)"
  hint: "Please use unittest.mock of Python 3 rather than plain/PyPI mock."

$ cat .pre-commit-config.yaml 
repos:
  - repo: https://github.com/codingjoe/relint
    rev: 1.0.0
    hooks:
      - id: relint

For config. Now it gets interesting:

$ git grep -E '^(import mock)' '*.py' | wc -l
72

$ pre-commit run --all-files | fgrep .py | wc -l
4

$ git ls-files '*.py' | xargs relint | fgrep .py | wc -l
4

So why are only 4 out of those 74 matches found by relint — any ideas?

codingjoe commented 5 years ago

@moseb thanks for reaching out.

I have an idea why your expression behaves that way. Relint matches the whole file, not line by line. This is the case, so people can write multi line expression. The leading ^ will therefore only match cases where the import is the very first line. If you limply make the pattern import mock, it should do what you expect. Please let me know if that resolves your issue.

Best, Joe

moseb commented 5 years ago

There is re.MULTILINE for the start of the string problem: https://docs.python.org/2/library/re.html#re.MULTILINE

When specified, the pattern character '^' matches at the beginning of the string and at the beginning of each line (immediately following each newline); and the pattern character '$' matches at the end of the string and at the end of each line (immediately preceding each newline). By default, '^' matches only at the beginning of the string, and '$' only at the end of the string and immediately before the newline (if any) at the end of the string.

Would adding re.MULTILINE be an option? The current behavior is rather unexpected and makes line based cases more difficult — (^|\n)import mock in my case. re.MULTILINE should allow both at the same time. What do you think?

codingjoe commented 5 years ago

@moseb that's a brilliant idea! You would add the flag in this line: https://github.com/codingjoe/relint/blob/f985128a53ef8d173222c2302085ed830c24409e/relint.py#L56

I would love it, would you conjure up a pull-request.