Skip to content

fix(src): resolve exclude paths#1034

Merged
monosans merged 2 commits intodjlint:masterfrom
antoineauger:fix/resolve-exclude-paths
Nov 27, 2024
Merged

fix(src): resolve exclude paths#1034
monosans merged 2 commits intodjlint:masterfrom
antoineauger:fix/resolve-exclude-paths

Conversation

@antoineauger
Copy link
Copy Markdown
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

@netlify
Copy link
Copy Markdown

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

Comment thread djlint/settings.py Outdated
@antoineauger
Copy link
Copy Markdown
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 thread djlint/settings.py Outdated
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
@monosans
Copy link
Copy Markdown
Member

Thank you!

monosans added a commit that referenced this pull request Nov 29, 2024
@monosans
Copy link
Copy Markdown
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
Copy Markdown
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).

@liambuchanan
Copy link
Copy Markdown

Whatever the solution ends up being here, it seems like it should probably use a new param name. Changing behavior for the existing param will cause issues for lots of users that have built workflows on the existing behavior. That's how I ended up here.

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

4 participants