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

feat: add build number editing for v1 recipes #3009

Merged
merged 10 commits into from
Sep 24, 2024
Merged

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Sep 24, 2024

No description provided.

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 12 lines in your changes missing coverage. Please review.

Project coverage is 76.26%. Comparing base (820dea3) to head (ebec146).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...forge_tick/update_recipe/v1_recipe/build_number.py 78.43% 11 Missing ⚠️
conda_forge_tick/migrators/core.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3009      +/-   ##
==========================================
+ Coverage   76.23%   76.26%   +0.03%     
==========================================
  Files         115      118       +3     
  Lines       12559    12628      +69     
==========================================
+ Hits         9574     9631      +57     
- Misses       2985     2997      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wolfv
Copy link
Contributor Author

wolfv commented Sep 24, 2024

Note: I added a fix to be able to pass a callable for new_build_number in #3011

IMO this could still be reviewed/merged separately and I could take care of managing the two PRs, as you prefer.

Copy link
Contributor

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I left a few comments. Also the module names of v1 and yaml are easy to stomp on. Let's

  • rename the v1 module to something longer like v1_recipes or w/e.
  • rename the yaml module to something else and possibly combine those routines into the ones in the utils module or the recipe parser module

@wolfv
Copy link
Contributor Author

wolfv commented Sep 24, 2024

I made the changes:

  • only touch build, build_number or number keys in context (and also only touches the first of them)
  • merged changes from the other PR to allow callables for build number
  • renamed to v1_recipe and got rid of the yaml module by re-using old code

Let me know if there is anything else

@wolfv
Copy link
Contributor Author

wolfv commented Sep 24, 2024

@beckermr - the 'safe' loader doesn't seem to be able to preserve the quotes - which one do we care about more?

@wolfv
Copy link
Contributor Author

wolfv commented Sep 24, 2024

Switched (back) to rt.

@wolfv
Copy link
Contributor Author

wolfv commented Sep 24, 2024

The initial idea behind ruamel.yaml was its RoundTripConstructor , which is also a subclass of the SafeConstructor. You used to get this using the now deprecated round_trip_load function and nowadays via the .load() method after using YAML(typ='rt'), this is also the default for a YAML() instance without typ argument. That RoundTripConstructor does not registers any tags or have any code that interprets tags above and beyond the normal tags that the SafeConstructor uses.

https://stackoverflow.com/a/71299116

@beckermr beckermr merged commit 228761a into regro:main Sep 24, 2024
5 checks passed
@wolfv wolfv deleted the build-number branch September 25, 2024 06:29
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.

2 participants