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: use a more specific regex for jinja2 set/for statements #5514

Merged
merged 11 commits into from
Oct 28, 2024

Conversation

beckermr
Copy link
Contributor

@beckermr beckermr commented Oct 23, 2024

Description

This PR uses a more specific regex for jinja2 set statements per the comment from @isuruf.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Oct 23, 2024
@beckermr
Copy link
Contributor Author

pre-commit.ci autofix

conda_build/variants.py Outdated Show resolved Hide resolved
conda_build/variants.py Outdated Show resolved Hide resolved
@beckermr beckermr changed the title fix: use a more specific regex for jinja2 set statements fix: use a more specific regex for jinja2 set/for statements Oct 23, 2024
@beckermr
Copy link
Contributor Author

pre-commit.ci autofix

isuruf
isuruf previously approved these changes Oct 23, 2024
@beckermr beckermr marked this pull request as ready for review October 23, 2024 16:18
@beckermr beckermr requested a review from a team as a code owner October 23, 2024 16:18
@beckermr beckermr enabled auto-merge (squash) October 23, 2024 16:19
@beckermr
Copy link
Contributor Author

Thanks for noticing and suggesting a fix @isuruf!

Copy link

codspeed-hq bot commented Oct 23, 2024

CodSpeed Performance Report

Merging #5514 will not alter performance

Comparing beckermr-patch-1 (c36a8c3) with main (810be76)

Summary

✅ 5 untouched benchmarks

@beckermr beckermr disabled auto-merge October 23, 2024 17:01
tests/requirements-ci.txt Outdated Show resolved Hide resolved
@beckermr
Copy link
Contributor Author

blocked by #5515

@beckermr beckermr marked this pull request as draft October 26, 2024 11:51
@beckermr
Copy link
Contributor Author

I think this will cause bugs with statements like {% set blah = var.upper() %}.

@wolfv
Copy link
Contributor

wolfv commented Oct 26, 2024

Can you not use real jinja functions for this?

@beckermr
Copy link
Contributor Author

See the comments in the code. We had this discussion.

@beckermr
Copy link
Contributor Author

Nvm. This works.

@beckermr beckermr marked this pull request as ready for review October 26, 2024 22:02
@beckermr
Copy link
Contributor Author

@isuruf @conda/builds-tools this is ready to merge!

@beckermr beckermr enabled auto-merge (squash) October 28, 2024 12:36
@beckermr beckermr merged commit 639be98 into main Oct 28, 2024
27 of 28 checks passed
@beckermr beckermr deleted the beckermr-patch-1 branch October 28, 2024 12:57
@beeankha beeankha mentioned this pull request Nov 15, 2024
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants