-
Notifications
You must be signed in to change notification settings - Fork 55
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
Filter out unmodified files from list of changed files #545
Conversation
Should this behavior maybe be the default? I mean, now that I think of it, if I just rename a source code file, why would I want the whole file to be reformatted in the same branch? |
I added it because you probably don’t want to check/format files unless their content has changed. Even when slowly phasing Black into a project. I made it off by default because the original behaviour was to not exclude these files. If you are happy to make it default and remove the option, it’d be easier as the argument can be removed and the git diff always filtered. If that is desired behaviour that is. Let me know what your thoughts are in this. |
For me (user) it makes sense that this is the default! |
1712f4f
to
5d07731
Compare
I made the changes in the last commit. Also added ‘A’ to the diff filter. |
aff3aa5
to
33f27e9
Compare
@Svenito, could you update the unit tests? See test failures in |
2ad3088
to
ea70ba8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
akaihola/darkgraylib#38 will be included in darkgraylib==1.1.0
. So you could rebase on master
, bump the dependency in setup.cfg
, and after I've made the release we can rerun the tests here.
Sorry for the large refactoring by moving stuff into the new Darkgraylib library. And thanks for noticing and reacting to it!
ea70ba8
to
55bf53f
Compare
@Svenito unfortunately you'll need to do one more rebase on Also I'll wait for at least one more backwards incompatible change to land before merging this one and bumping the major version number. So probably later in the spring we'll get this one into a release. Thanks a lot for your effort and sorry for the delay! |
Sounds good to me. No problem on the delay either. |
55bf53f
to
78fda22
Compare
Hi, just wanted to check if there's a rough ETA on this patch making it in? |
I guess it's starting to be time to bring this in. There are no significant backwards-incompatible changes to bundle with it in v3.0.0, so maybe we'll just go with this one and risk needing to bump to v4 sooner than later. So let me start prepare for a v3.0.0 release. |
78fda22
to
a0607ec
Compare
d230145
to
7457498
Compare
7457498
to
e5286e8
Compare
Add `--diff-filter=M` to the git command that fetches changed files. The filter will ignore files that have been renamed or moved, but not had their content changed
Adding ``--diff-filter=M` will only show modified files, but darker should also check newly added files. Add the `A` option to the diff call to include these
With the added diff filter some tests needed updating to reflect the change
* New line in test to separate test from verification
e5286e8
to
8daad20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to merge to me now. I don't understand why 13 CI jobs are always left unrun. But I did those lints locally, all is good.
Adds an option--modified-only/-m
to ignore renamed files from being checked.Make the filter the default behaviour. Possibly add an option to include renamed files or not, depending on discussions below
Fixes #544