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

Update convert to properly handle complex Jinja expressions #176

Merged
merged 11 commits into from
Oct 9, 2024

Conversation

beeankha
Copy link
Member

@beeankha beeankha commented Oct 2, 2024

Resolves #166

@beeankha beeankha added the bug Something isn't working label Oct 2, 2024
@beeankha beeankha self-assigned this Oct 2, 2024
@@ -116,7 +116,7 @@ class Regex:
"""

# Pattern to detect Jinja variable names and functions
_JINJA_VAR_FUNCTION_PATTERN: Final[str] = r"[a-zA-Z_][a-zA-Z0-9_\|\'\"\(\)\[\]\, =\.\-]*"
_JINJA_VAR_FUNCTION_PATTERN: Final[str] = r"[a-zA-Z_][a-zA-Z0-9_\|\'\"\(\)\[\]\, =\.\-~]*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually don't see ~ in the proposed CEP-17, so I'm not sure if it will be supported.
I also need to figure out what the hell that actually does in JINJA, lol.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay so from context of the original post, I think the ~ is tied to semantic versioning. Now why that's valid outside of the string argument in the pin_subpackage() example, I'm less sure.

@beeankha beeankha changed the title Update convert to handle ~ Jinja expression Update convert to properly handle complex Jinja expressions Oct 3, 2024
self._msg_tbl.add_message(
MessageCategory.WARNING,
f"Complex Jinja expressions detected in key(s): {complex_jinja_display}\n"
f"The following syntax cannot be automatically converted: {complex_jinja_pattern_display}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would simplify this into one warning line. I don't think we have to show the regex pattern, just the key associated with the failure.

@beeankha beeankha marked this pull request as ready for review October 9, 2024 18:36
@beeankha beeankha requested a review from a team as a code owner October 9, 2024 18:36
Copy link
Collaborator

@schuylermartin45 schuylermartin45 left a comment

Choose a reason for hiding this comment

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

Good job! We might have to iterate on this as we learn more about these kinds of edge cases. But, this is a good start!

@schuylermartin45 schuylermartin45 merged commit 8f659de into conda-incubator:main Oct 9, 2024
14 checks passed
@schuylermartin45
Copy link
Collaborator

schuylermartin45 commented Oct 9, 2024

If you compare with the last integration test against main, this fixes compatibility with 6 recipes. Nice!

ForgottenProgramme pushed a commit that referenced this pull request Nov 6, 2024
* Add tilde to _JINJA_VAR_FUNCTION_PATTERN

* Create V0_FORBIDDEN_JINJA constant and check for strings containing them when converting recipes

* Add exception for when Jinja syntax that is too complex to convert is detected

* Make the linter happy

* Change name of V0_UNSUPPORTED_JINJA constant

* Log 'complex Jinja' warning with the MessageTable instance instead of throwing an exception

* Add more unsupported Jinja to regression_jinja_sub.yaml test recipe file to catch new warning in test

* Simplify warning message and update related test
schuylermartin45 added a commit that referenced this pull request Nov 6, 2024
* add bump-recipe command

* fix type error

* add test

* fix failing tests

* Update conda_recipe_manager/commands/bump_recipe.py

Co-authored-by: Schuyler Martin <schuylermartin45@gmail.com>

* add some suggestions from the code review

* more changes based on previous code review

* check if the build number was actually incremented

* parametrize the test

* Update tests/commands/test_bump_recipe.py

Co-authored-by: Schuyler Martin <schuylermartin45@gmail.com>

* Update tests/commands/test_bump_recipe.py

Co-authored-by: Schuyler Martin <schuylermartin45@gmail.com>

* Update tests/commands/test_bump_recipe.py

Co-authored-by: Schuyler Martin <schuylermartin45@gmail.com>

* Update tests/commands/test_bump_recipe.py

Co-authored-by: Schuyler Martin <schuylermartin45@gmail.com>

* Update tests/commands/test_bump_recipe.py

Co-authored-by: Schuyler Martin <schuylermartin45@gmail.com>

* Update `convert` to properly handle complex Jinja expressions (#176)

* Add tilde to _JINJA_VAR_FUNCTION_PATTERN

* Create V0_FORBIDDEN_JINJA constant and check for strings containing them when converting recipes

* Add exception for when Jinja syntax that is too complex to convert is detected

* Make the linter happy

* Change name of V0_UNSUPPORTED_JINJA constant

* Log 'complex Jinja' warning with the MessageTable instance instead of throwing an exception

* Add more unsupported Jinja to regression_jinja_sub.yaml test recipe file to catch new warning in test

* Simplify warning message and update related test

* Fixes #186: Parser cannot handle addition in JINJA replacements (#191)

* Fixes #186 and adds some support for JINJA addition/concatenation

* Adds unit tests for JINJA addition/concatentation evaluations

* Python code Dependency Scanner Prototype (#180)

* Initial blueprint for dependency scanning module

* Starts to adapt work from abandonned dependency CLI branch

* Finishes initial work on pulling module names as dependencies

* Adds local module filtering

* Adds dependency type support to PythonDependencyScanner

* Adds MessageTable instance to base dependency class

* Adds missing docs

* Minor fixes

* Adds note about scanning performance

* Addresses linter and analyzer issues

* Adds initial dependency scanning unit test

* Adds ignore line to dummy project file

* Improves unit test coverage

* Updates recipe file to ignore test data directory

* Adds support for multiple imports on one line

* Add __init__ files to all test directories (#192)

* v0.3.0 commit (#193)

* Forgot to include some minor changes in the v0.3.0 release commit (#194)

* Updates the recipe doc link (#195)

* Fixes #197 and adds regression tests. Partially fixes #190 (#199)

* Removes missed debug statement (#200)

* 🤖 updated file(s) (#201)

* v0.3.1 (#202)

* Removes conda-forge channel (#204)

* Test the `MessageTable` class (#196)

* Add test for all methods in MessageTable class from types.py

* Split up/parametrize MessageTable unit tests

* Parametrize the MessageTable class tests and expand test cases

* Correct doc string formatting

* 207 Refactor `RecipeParserDeps` (#209)

* Renames RecipeParserDeps -> RecipeReaderDeps

* Renames RecipeParserDeps -> RecipeReaderDeps

* Fleshes-out initial utility functions in the new RecipeParserDeps class

* Improves SelectorParser capabilities

* Adds selector management to the add_dependency() function

* Adds unit test for selector rendering

* Added SelectorParser type variant to test_add_selector()

* Adds initial RecipeParserDeps unit tests

* Adds initial multi-output add_dependency() test variants

* Adds unit tests for remove_dependency()

* Adds invalid path tests to test_add_dependency()

* Adds alternative DependencyMode unit tests to test_add_dependency()

* Adds missing invalid path checks for test_add_dependency()

* Adds more edge-case unit tests to test_add_dependency()

* Fixes patch-add append edge case

* Adds missing unit test for adding a dependency to a non-existent dependency section

* Adds another missing unit test for adding a dependency to a non-existent dependency section

* v0.3.2 (#210)

* Fixes `numpy {{numpy}}` dependency bug (#214)

* Fixes resilience issue with dependency_data_from_str() function also makes function names more consistent

* Adds missing unit tests for dependency module

* Removes outdated TODO comment

* Adds regression testing for the numpy dependency parsing issue

* v0.3.3 (#216)

* v0.3.4 (#217)

* Improves pytest config filter to be less aggressive (#223)

* 🤖 updated file(s) (#226)

* Incorporating user documentation feedback and simplifies the `Makefile` (#227)

* Ignores another pytest deprecation

* Simplifications and deprecations for the Makefile. The preferred method for installing CRM is through conda/conda-forge

* Overhauls existing README documentation

* Updates README TOC

* More README doc work

* More README improvements

* Filling more doc gaps

* Simplifies and automates the API documentaiton process

* Update README.md

Co-authored-by: Mahe Iram Khan <65779580+ForgottenProgramme@users.noreply.github.com>

* Update README.md

Co-authored-by: Mahe Iram Khan <65779580+ForgottenProgramme@users.noreply.github.com>

* Update README.md

Co-authored-by: Mahe Iram Khan <65779580+ForgottenProgramme@users.noreply.github.com>

* Adds a contribution file

* Update CONTRIBUTING.md

---------

Co-authored-by: Mahe Iram Khan <65779580+ForgottenProgramme@users.noreply.github.com>

* remove dangling else statements

---------

Co-authored-by: Schuyler Martin <schuylermartin45@gmail.com>
Co-authored-by: Bianca Henderson <bhenderson@anaconda.com>
Co-authored-by: conda-bot <18747875+conda-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

convert does not handle some jinja expressions
3 participants