Design review to allow underscore in event name #2232
Conversation
|
|
sagarvish1989
had a problem deploying
to
external
GitHub Actions
Failure
github-actions
bot
added
the
comp:exporter.onecollector
| @@ -54,7 +65,8 @@ public static bool IsEventNameValid(string eventName) | |||
|
|
|||
| public ResolvedEventFullName ResolveEventFullName( | |||
| string? eventNamespace, | |||
| string? eventName) | |||
| string? eventName, | |||
| EventNameDelimiter eventNameDelimiter = EventNameDelimiter.Period) | |||
There was a problem hiding this comment.
Yeah, not needed. Will remove.
| @@ -124,7 +136,8 @@ private static byte[] BuildEventFullName(string eventNamespace, string eventName | |||
| { | |||
| WriteEventFullNameComponent(eventNamespace, destination, ref cursor); | |||
|
|
|||
| destination[cursor++] = (byte)'.'; | |||
| byte delimiter = eventNameDelimiter == EventNameDelimiter.Period ? (byte)'.' : (byte)'_'; | |||
| @@ -112,7 +124,7 @@ public ResolvedEventFullName ResolveEventFullName( | |||
| return resolvedEventFullName; | |||
| } | |||
|
|
|||
| private static byte[] BuildEventFullName(string eventNamespace, string eventName) | |||
| private static byte[] BuildEventFullName(string eventNamespace, string eventName, EventNameDelimiter eventNameDelimiter = EventNameDelimiter.Period) | |||
There was a problem hiding this comment.
Probably by the time this function is reached, the eventNameDelimiter should be a byte and not an enum
There was a problem hiding this comment.
Actually, unless we really want this to be settable on a per-event basis, this shouldn't be a parameter - you should just reference the property of the EventNameManager
| private static readonly Regex EventNamespaceValidationRegex = new(@"^[A-Za-z](?:\.?[A-Za-z0-9]+?)*$", RegexOptions.Compiled); | ||
| private static readonly Regex EventNameValidationRegex = new(@"^[A-Za-z][A-Za-z0-9]*$", RegexOptions.Compiled); | ||
|
|
||
| private readonly string defaultEventNamespace; | ||
| private readonly string defaultEventName; | ||
| private readonly EventNameDelimiter defaultEventNameDelimiter; |
There was a problem hiding this comment.
I think this should just be 'eventNameDelimiter'.
The 'defaultEventNamespace' and 'defaultEventName' properties are about the values for a default event, not the default values for an arbitrary event
There was a problem hiding this comment.
That is - someone could call log(dataFields: foo) without passing an event name, and the event name would be the default event name. (pseudocode, something like that)
|
@sagarvish1989, could you please sing EasyCLA? |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Trying to reopen the PR to make it active |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Fixes #
Design discussion issue #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes