-
Notifications
You must be signed in to change notification settings - Fork 8
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
chore: refactor to put function name generator in constant #641
Conversation
@@ -598,7 +600,7 @@ func Error_ToDafny(err error)($L.Error) { | |||
} | |||
""", | |||
DafnyNameResolver.dafnyTypesNamespace(serviceShape), | |||
writer.consumer(w -> { | |||
writer.consumer(w -> { |
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.
Not sure how this came in diff but make format_java will fix this. So, Ignoring this for 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.
Personally I'm not a fan of our java formatter - it keeps line too short and I think uses 2 white-spaces for indentation.
@@ -807,7 +809,7 @@ private void generateSerializerFunctions(final GenerationContext context, final | |||
return $L | |||
} | |||
""", | |||
ShapeVisitorHelper.funcNameGenerator( |
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.
nit: I'd prefer fully-qualified-name / FQN over static imports. Improves readability / reasoning around where this function belongs.
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.
Done
@@ -598,7 +600,7 @@ func Error_ToDafny(err error)($L.Error) { | |||
} | |||
""", | |||
DafnyNameResolver.dafnyTypesNamespace(serviceShape), | |||
writer.consumer(w -> { | |||
writer.consumer(w -> { |
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.
Personally I'm not a fan of our java formatter - it keeps line too short and I think uses 2 white-spaces for indentation.
) { | ||
return memberShape | ||
.getId() | ||
.toString() | ||
.replaceAll("[.$#]", "_") | ||
.concat("_") | ||
.concat(suffix); |
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.
Why is this different than one in the ValidationGenerator
?
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 thought the name in ValidationGenerator won't collide and will look good but the name collided (Caught in CI in 8738b87) So, I made ValidationGenerator to use same function name.
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.
Follow this up with the formatting fix.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.