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

Filter out unmodified files from list of changed files #545

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Svenito
Copy link
Collaborator

@Svenito Svenito commented Mar 5, 2024

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

@akaihola
Copy link
Owner

akaihola commented Mar 5, 2024

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?

@akaihola akaihola self-requested a review March 5, 2024 20:37
@akaihola akaihola assigned akaihola and Svenito and unassigned akaihola Mar 5, 2024
@akaihola akaihola added the enhancement New feature or request label Mar 5, 2024
@Svenito
Copy link
Collaborator Author

Svenito commented Mar 5, 2024

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.

@Timple
Copy link

Timple commented Mar 5, 2024

For me (user) it makes sense that this is the default!

@Svenito Svenito changed the title Add modified only option Filter out unmodified files from list of changed files Mar 6, 2024
@akaihola
Copy link
Owner

akaihola commented Mar 7, 2024

Thanks @Timple, and indeed @Svenito let's remove the option and hard-code this behavior as the default. I'll bump the major version number before including this in a release. Thanks!

@Svenito
Copy link
Collaborator Author

Svenito commented Mar 7, 2024

I made the changes in the last commit. Also added ‘A’ to the diff filter.

@akaihola
Copy link
Owner

akaihola commented Mar 8, 2024

@Svenito, could you update the unit tests? See test failures in test_git.py and test_main_revision.py. Let me know if you need me to clarify how the tests work.

Copy link
Owner

@akaihola akaihola left a 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!

@akaihola
Copy link
Owner

@Svenito unfortunately you'll need to do one more rebase on master before the tests have a chance to pass.

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!

@Svenito
Copy link
Collaborator Author

Svenito commented Mar 15, 2024

Sounds good to me. No problem on the delay either.

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
* remove empty conftest.py file
@Svenito Svenito requested a review from akaihola March 18, 2024 15:38
@Svenito
Copy link
Collaborator Author

Svenito commented May 8, 2024

Hi, just wanted to check if there's a rough ETA on this patch making it in?

@akaihola
Copy link
Owner

akaihola commented May 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Add option to ignore moved/deleted files
3 participants