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

Resolve #8 -- Add RegEx based filePattern attribute in favor of filename #11

Merged
merged 2 commits into from
Nov 11, 2019

Conversation

codingjoe
Copy link
Owner

No description provided.

@codecov
Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #11 into master will decrease coverage by 2.29%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #11     +/-   ##
=========================================
- Coverage   95.87%   93.57%   -2.3%     
=========================================
  Files           1        1             
  Lines          97      109     +12     
=========================================
+ Hits           93      102      +9     
- Misses          4        7      +3
Impacted Files Coverage Δ
relint.py 93.57% <83.33%> (-2.3%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01f4cc0...c5d9f07. Read the comment docs.

@codingjoe
Copy link
Owner Author

@anapaulagomes I'd also like your feedback here. The issue came up, because glob doesn't allow exclusion. I believe it to be better to just ditch glob and use regex everywhere. After all this is a regex linter. I believe users will be able to cope.

@anapaulagomes
Copy link
Collaborator

Sure, makes sense. I'm on it.

Copy link
Collaborator

@anapaulagomes anapaulagomes left a comment

Choose a reason for hiding this comment

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

Nice feature! I've tested in a big repo (😇) but I'm not sure if I got the examples right. After this first round and clarification about the pattern, I'll test it again.

README.rst Outdated Show resolved Hide resolved
relint.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated
- "**/test_*.py"
- "conftest.py"
- "**/conftest.py"
filePattern: "(test_.*|conftest)\.py"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
filePattern: "(test_.*|conftest)\.py"
filePattern: "(test_.*|conftest)\\.py"

Copy link
Owner Author

@codingjoe codingjoe Oct 29, 2019

Choose a reason for hiding this comment

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

You are right, in JSON backslashes need to be escaped. In YAML however they don't:

>>> import re, yaml
>>> re.compile(yaml.load('1: \\.')[1]).search('test.png')
<re.Match object; span=(4, 5), match='.'>
>>> re.compile(yaml.load('1: \.')[1]).search('test.png')
<re.Match object; span=(4, 5), match='.'>

To be honest I wasn't aware of that myself, after reading more into it, I see that we could write plain old RegEx into the YAML config. I wonder if we should change all example to reflect that. I know that the quoting makes the examples even harder for some to understand. What do you think @anapaulagomes

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
.relint.yml Outdated Show resolved Hide resolved
relint.py Show resolved Hide resolved
@codingjoe codingjoe force-pushed the filename branch 3 times, most recently from 8722927 to 1585e9e Compare October 29, 2019 01:20
@codingjoe
Copy link
Owner Author

@anapaulagomes I rewrote all patterns to use either none or single quotes where needed. As a result we no longer need to JSON escape characters. I personally think it's a lot more readable. Check out the new README, what do you think?

BTW, thanks for the review, much appreciated 🥇

@anapaulagomes
Copy link
Collaborator

I'll take a look soon, @codingjoe! 🙏

Copy link
Collaborator

@anapaulagomes anapaulagomes left a comment

Choose a reason for hiding this comment

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

🏅 nice!

Please note that model_mommy was deprecated in favor of model_bakery. There is a script to help on the migration from mommy to baker in the new repo.

@codingjoe codingjoe merged commit 3bd5a23 into master Nov 11, 2019
@codingjoe codingjoe deleted the filename branch November 11, 2019 09:45
@codingjoe
Copy link
Owner Author

Released in 1.2.0 🎉

Please note that model_mommy was deprecated in favor of model_bakery. There is a script to help on the migration from mommy to baker in the new repo.

@anapaulagomes cool! I head from @amureki that the two of you have been working on a new stable release, nice 🥇 Would you mind updating the example in the README.md to match the new syntax? Since I haven't been at work, I haven't used mommy, yet.

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.

2 participants