-
Notifications
You must be signed in to change notification settings - Fork 153
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
Config option: strip-enum-member-type-name #527
Config option: strip-enum-member-type-name #527
Conversation
Sorry for the delay on this, I've been heads down on some dotnet/runtime related work and completely forgot to respond. This is on my backlog (as are the other couple PRs) to finish reviewing and get them merged. At a high level glance, they all looked reasonable and I just need to sit down and finish going through all the code. |
if (Config.StripEnumMemberTypeName) | ||
{ | ||
Regex stripParentNameRegex = new($"^{Regex.Escape(parentName)}_*", RegexOptions.IgnoreCase); | ||
var strippedName = stripParentNameRegex.Replace(escapedName, string.Empty); | ||
escapedName = strippedName; | ||
} |
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.
I don't think this needs to be/should be a regex. We're really not doing anything fancy here.
This can really just be like EscapeAndStripName
is doing, where we're just doing:
private string EscapeAndStripEnumConstantName(string name)
{
if (name.StartsWith(parentName, StringComparison.Ordinal))
{
var startIndex = parentName.Length;
if ((name.Length > startIndex) && (name[startIndex] == '_'))
{
startIndex += 1;
}
name = name[startIndex..];
}
return EscapeName(name);
}
-- EscapeAndStripName
should likely be renamed to EscapeAndStripMethodName
, a similar change should be made to PrefixAndStripName
since it also only applies to methods today.
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.
@tannergooding I have created a new general-purpose method PrefixAndStrip
and made the enum stripping logical call that. I have also made EscapeAndStripName
and PrefixAndStripName
call that new method as well and renamed them as you suggested.
See commit 6703d6a
c32a0c5
to
e94f046
Compare
e94f046
to
c246b41
Compare
@@ -294,6 +295,11 @@ private void VisitEnumConstantDecl(EnumConstantDecl enumConstantDecl) | |||
parentName = _outputBuilder.Name; | |||
} | |||
|
|||
if (Config.StripEnumMemberTypeName) | |||
{ | |||
escapedName = PrefixAndStrip(escapedName, parentName, trimChar: '_'); |
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.
Sorry for the delay, was busy at Microsoft Build.
The rest of the changes look good, but trimChar: '_'
is going to cause some problems and needs an extra handler. In particular we know that there exist enums of the form DXGI_FORMAT_420_OPAQUE
and so removing the prefix DXGI_FORMAT
+ _
will cause an invalid member name to be generated.
I'm going to merge this as is given that these are rare and there is a trivial workaround where devs can simply remap that specific member. However, if you have time it'd be great to at least add in a diagnostic when such a member is encountered and possibly add back the leading _
to ensure we still get "valid" codegen.
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.
Sure thing, Looks like we first need a check for leading digits (i think that's the only problem from the subset of what's valid in C/C++ and C# at that point) and then another call to EscapeName in case we ended up with a keyword after stripping.
I'll make the changes.
In case of leading digits it's fine to just prepend with @ like EscapeName does?
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.
In case of leading digits it's fine to just prepend with @ like EscapeName does?
Unfortunately no, @
only works on valid identifiers and is really just to remove ambiguity with respect to language keywords. You'd have to reinsert a leading _
to make it valid if the first character triggers the char.IsDigit
check.
Adds a new configuration option
strip-enum-member-type-name
. This feature adresses the feature request described in issue #461.When the config option is applied, the enum type name is stripped from the beginning of each enum member.
becomes
Note above, that the original member name starts with
abc_some_enum_
which matches the enum type nameabc_some_enum
separating underscores between the type name and the suffix are also stripped to avoid leading underscores in the member names.The matching for the enum type name is only done if the type appears verbatim in the beginning of the member name. Infix or suffix matching of the type name is not supported.
As @tannergooding notes in #461 (comment), there are other solutions to make C-language stype enum be nicely usable in C#. However, the different approaches are ultimately stylistic choices and therefore I argue that this feature makes sense as a configuration option, allowing users to choose freely between styles. Following that argument, the option is kept simply to apply to the entire code generation, i.e. generation for stripping one enum, but not another is not supported as that would lead to two different styles within the same generated codebase.