-
Notifications
You must be signed in to change notification settings - Fork 655
Sanitize Participant #4588
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
base: main
Are you sure you want to change the base?
Sanitize Participant #4588
Conversation
cfa4003
to
76fb48d
Compare
76fb48d
to
37c11c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing this and providing a fix! By the test cases, it seems like a simple .RegexReplace("[^a-zA-Z0-9]", "_")
would do the trick – am I missing something? :)
GitVersion/src/GitVersion.Core/Extensions/StringExtensions.cs
Lines 14 to 18 in f8a95ab
public static string RegexReplace(this string input, string pattern, string replace) | |
{ | |
var regex = RegexPatterns.Cache.GetOrAdd(pattern); | |
return regex.Replace(input, replace); | |
} |
Don't let perfection be enemy of good enough; fair. I'm a little OCD when it comes to clean consistency so my initial proposal's a bit opionated; but also cleaner IMHO but the goals to be able to copy paste in to PlantUml and get an output. There are likely other curiousities i'd not encountered so the simplicity of the RegEx you propose wouldn't go amiss. If you're happy to take one or other; or both options; let me know how to tidy up the PR to meet acceptable standards to get a merge. Thanks for consideration.
|
37c11c0
to
aaf3c3e
Compare
aaf3c3e
to
ba750dd
Compare
So having circled back to use the output in earnest I'm seeing that the simple Regex really would be sufficient due to the @as mechanism that had already been implemented. The sanitized participant doesn't appear in the ui anyhow, the feature/f1 maps to feature_f1 so it doesn't break PlantUml but the original feature/f1 is still displayed. FWIW I see what I was missing now 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I really appreciate the level of investment, but I think this does a bit much to solve a relatively simple problem. Since, as you write, @as
takes care of the human readability aspect of the diagram, I don't think the programmatic name in the diagram needs this level of sophistication. :)
|
||
namespace GitVersion.Testing.Helpers; | ||
|
||
public static partial class RegexReplacer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the partial class
or could the NonAlphanumericRegex
method just be jammed into the (already existing) StringExtensions
class?
@@ -39,11 +40,12 @@ public SequenceDiagram() | |||
/// </summary> | |||
public void Participant(string participant, string? @as = null) | |||
{ | |||
this.participants.Add(participant, @as ?? participant); | |||
if (@as == null) | |||
var cleanParticipant = ParticipantSanitizer.SanitizeParticipant(@as ?? participant); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we not use the RegexReplace()
method here directly?
var cleanParticipant = ParticipantSanitizer.SanitizeParticipant(@as ?? participant); | |
var cleanParticipant = (@as ?? participant).RegexReplace(…); |
🧵 Comparing
|
Use Case | Recommended Approach |
---|---|
Known, constant patterns | GeneratedRegex |
Dynamic or user-defined patterns | Runtime + caching (RegexPatterns.Cache ) |
Performance-critical code | GeneratedRegex |
Multi-line, complex patterns | Traditional regex + IgnorePatternWhitespace |
Needs AOT compatibility | GeneratedRegex (strongly preferred) |
If you prefer I can just follow the RegexPatterns convention for consistency, but especially for the preexisting as you've dropped net6 I'd strongly suggest converting as they're heavily used in the calculation so the compile time optimization would be worthwhile?
Let me know which route to go and I'll rip out the initial one, leave only regex.
As long as all tests pass, I'm all for converting to the new, precompiled variant! 🙂👍🏼 |
There is this class https://github.com/GitTools/GitVersion/blob/main/src/GitVersion.Core/Core/RegexPatterns.cs where we have all our regex, we will migrate this to newer GeneratedRegex in one go later on |
Description
The SequenceDiagrams generated by the ComplexGitflowExample were invalid because the participants were incompatible with PlantUml. I've sanitized them so the SequenceDiagrams output by tests are cut & paste viewable.
Related Issue
#4585
Motivation and Context
So the SequenceDiagrams output by tests are cut & paste viewable.
How Has This Been Tested?
Test coverage and output of ComplexGitflowExample is now cut-paste viewable.
Screenshots (if appropriate):
For example feature/f1 is invalid - participant can't have '/' in it.
Checklist: