Skip to content

Convert to GeneratedRegexes #4595

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

Merged
merged 3 commits into from
Jun 27, 2025
Merged

Convert to GeneratedRegexes #4595

merged 3 commits into from
Jun 27, 2025

Conversation

9swampy
Copy link
Contributor

@9swampy 9swampy commented Jun 24, 2025

Description

Convert to GeneratedRegexes

Related Issue

Motivation and Context

#4588 (comment) As long as all tests pass, I'm all for converting to the new, precompiled variant!

#4588 (comment) we will migrate this to newer GeneratedRegex in one go later on

How Has This Been Tested?

Primarily deferring to existing test coverage but I retrospectively went back and added https://github.com/GitTools/GitVersion/pull/4595/files#diff-05aaddf75a2990dace1f1b29f0c7bdccfab7d710e2e4b0c94a168020a5b9e5e4
as AI had suggested some dubious conversions.

Splitting up the commits helped build confidence in the conversions; good shout @arturcic

Without NCrunch isn't so easy for me to validate coverage but presuming your tooling does make it easy? Point me to any gaps that concern and I could take a look.

Screenshots (if appropriate):

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@9swampy 9swampy marked this pull request as ready for review June 25, 2025 01:37
@arturcic
Copy link
Member

@9swampy thanks for creating this PR, it's a nice follow up to the effort to have a consistent common RegexPatterns class. I never had the time to continue the effort. Thanks for adding tests to validate the regex patterns prior to other changes. Please have a look at the comments I added.

@9swampy 9swampy requested a review from arturcic June 25, 2025 20:28
@9swampy
Copy link
Contributor Author

9swampy commented Jun 25, 2025

DictionaryExtensions.GetAdd<string, Regex> no longer referenced but it's in the shipped API, I'd suggest it's better to hide the Cache dictionary altogether?

@arturcic
Copy link
Member

DictionaryExtensions.GetAdd<string, Regex> no longer referenced but it's in the shipped API, I'd suggest it's better to hide the Cache dictionary altogether?

yes, remove the extension, and in the same class there is one more GetAdd, you probably can mark it as internal, then you can remove the entries from the publicAPI (the builld should fail if not removed I guess)

@arturcic
Copy link
Member

you will need to rebase onto main as well

@arturcic
Copy link
Member

arturcic commented Jun 25, 2025

In a scope but not for this PR. I know in the dotnet 9 they expanded GeneratedRegex to support partial properties as well. Maybe you can give it a try to check how ugly will be to have for dotnet8 the partial method impl and for dotnet9 partial property? We can also wait until dotnet 8 is EOL and then we can have only the partial property variant. Mind to give it a try?

@arturcic
Copy link
Member

@9swampy can you please rebase onto main one last time, and then I can merge

Copy link
Member

@arturcic arturcic left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@arturcic arturcic enabled auto-merge June 27, 2025 08:19
@arturcic arturcic merged commit 2f439aa into GitTools:main Jun 27, 2025
93 checks passed
Copy link
Contributor

mergify bot commented Jun 27, 2025

Thank you @9swampy for your contribution!

@9swampy
Copy link
Contributor Author

9swampy commented Jun 27, 2025

Nice to give a little back; I've been using GitVersion since Jake gave a demo at a London meetup very early doors so long overdue. Kudos for keeping it moving along.

Code wise it's a no brainer but I'd be curious if you extract any performance stats the see if the change made much difference?

@arturcic
Copy link
Member

Honestly I did not notice much performance on the repos I tested mainly because they are not that big. But this should server in the potential effort going towards native AOT

@arturcic
Copy link
Member

Nice to give a little back; I've been using GitVersion since Jake gave a demo at a London meetup very early doors so long overdue. Kudos for keeping it moving along.

Thanks for wanting to put some effort on this project.

@9swampy 9swampy deleted the ConvertRegexes branch June 28, 2025 22:22
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