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

fix(src): resolve exclude paths #1034

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

antoineauger
Copy link
Contributor

@antoineauger antoineauger commented Nov 22, 2024

Pull Request Check List

Resolves: #issue-number-here

  • Adapted tests for changed code.
  • Updated documentation for changed code.

Closes #1028

🛠️ with ❤️ by Siemens

/cc @nejch

Copy link

netlify bot commented Nov 22, 2024

Deploy Preview for djlint canceled.

Name Link
🔨 Latest commit 1ccc345
🔍 Latest deploy log https://app.netlify.com/sites/djlint/deploys/67409cb006ec170008758b99

djlint/settings.py Outdated Show resolved Hide resolved
@antoineauger
Copy link
Contributor Author

⚠️ This PR would theoretically be a breaking change since previously-define exclude patterns might not work with the new Path resolve.

The documentation seems to already be up-to-date with the behavior of this MR though:

  --exclude TEXT                  Override the default exclude paths.
  --extend-exclude TEXT           Add additional paths to the default exclude.

Let me know what you think, I will be happy to adapt/refactor if needed.

Comment on lines 1128 to 1131
exclude_paths = []
for p in self.exclude.split("|"):
exclude_paths.append(Path(p.strip().replace("\\.", ".")).resolve())
return exclude_paths
Copy link

Choose a reason for hiding this comment

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

Not sure but maybe this could also just be a comprehension :)

Suggested change
exclude_paths = []
for p in self.exclude.split("|"):
exclude_paths.append(Path(p.strip().replace("\\.", ".")).resolve())
return exclude_paths
return [Path(p.strip().replace("\\.", ".").resolve() for p in self.exclude.split("|")]

Copy link
Member

Choose a reason for hiding this comment

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

Or even better a for loop with yield (generator)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made another attempt and pushed 1ccc345

@antoineauger antoineauger force-pushed the fix/resolve-exclude-paths branch from d85dbe2 to 1d29cf0 Compare November 22, 2024 14:29
@antoineauger antoineauger marked this pull request as ready for review November 22, 2024 14:30
@antoineauger antoineauger force-pushed the fix/resolve-exclude-paths branch from 1d29cf0 to aecb771 Compare November 22, 2024 14:39
@antoineauger antoineauger force-pushed the fix/resolve-exclude-paths branch from aecb771 to 871a75e Compare November 22, 2024 14:53
@monosans monosans merged commit bccc364 into djlint:master Nov 27, 2024
8 checks passed
@monosans
Copy link
Member

Thank you!

monosans added a commit that referenced this pull request Nov 29, 2024
@monosans
Copy link
Member

Hi, I have reverted this PR as it is a breaking change and I think it should be reworked. The old behavior was more correct imo.

@antoineauger
Copy link
Contributor Author

Hi @monosans, sorry to read that.

as it is a breaking change

Yes, I mentioned it in #1034 (comment).

I think it should be reworked

I just saw #1047 and I might have overlooked some cases, sorry about that. But given that this exclude is a pretty standard option present in a lot of linters, I'm sure that this can be properly solved without too much hacking 😅

The old behavior was more correct imo.

I tend disagree on this point.

If --exclude=build is passed, all paths containing *build* will be ignored, even if the string only partially match a parent directory. For instance, /builds/template.html.j2, /build-my-template/template.html.j2, etc. will always be ignored no matter where djlint is ran.

This causes major problems for people in CI with GitLab for instance (our case).

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.

[BUG] [Linter] djlint exclude option does not work as expected
3 participants