-
Notifications
You must be signed in to change notification settings - Fork 60
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
[jnigen] adding the ability for the user to rename #1946
[jnigen] adding the ability for the user to rename #1946
Conversation
commit 92d2a8d Author: Marshelino Maged <[email protected]> Date: Mon Jan 27 21:17:25 2025 +0200 ability for the user to rename commit eec4f6e Author: Marshelino Maged <[email protected]> Date: Mon Jan 27 20:38:36 2025 +0200 edit test commit 24453db Author: Marshelino Maged <[email protected]> Date: Mon Jan 27 19:30:11 2025 +0200 enable user to rename
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
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.
LGTM with one very small change.
log.fine('Class ${node.binaryName} is named ${node.finalName}'); | ||
|
||
if (node.userDefinedName == null || | ||
node.userDefinedName == node.finalName) { |
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.
Exactly!
@@ -227,7 +235,8 @@ class _MethodRenamer implements Visitor<Method, void> { | |||
|
|||
@override | |||
void visit(Method node) { | |||
final name = _preprocess(node.isConstructor ? 'new' : node.name); | |||
final name = _preprocess( | |||
node.isConstructor ? 'new' : node.userDefinedName ?? node.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.
The user should technically be able to rename the constructors as well, I'm not sure that we currently support this in the dart_generator though. So let's fix that in another PR.
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.