-
Notifications
You must be signed in to change notification settings - Fork 45
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
WASM Automatic Demo Generator Proof of Concept #500
base: main
Are you sure you want to change the base?
WASM Automatic Demo Generator Proof of Concept #500
Conversation
Got classes in place, but we need a workaround.
Mixed syntax for arguments, terminusArgs, and ...args. Cleaning up would be helpful, so using the argument names would be better than |
Move |
null, | ||
[ | ||
terminusArgs[0] | ||
] | ||
), |
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: Indentation?
(function (...args) { | ||
let out = new ICU4XFixedDecimalFormatterOptions(); | ||
|
||
out.groupingStrategy = args[0]; | ||
|
||
out.someOtherConfig = args[1]; | ||
|
||
return out; | ||
}).apply( |
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: mixed tabs and spaces
(function (...args) { | ||
let out = new ICU4XFixedDecimalFormatterOptions(); | ||
|
||
out.groupingStrategy = args[0]; | ||
|
||
out.someOtherConfig = args[1]; | ||
|
||
return out; | ||
}).apply( | ||
null, | ||
[ | ||
terminusArgs[1], | ||
terminusArgs[2] | ||
] | ||
) |
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.
Suggestion, if easy:
(function (...args) { | |
let out = new ICU4XFixedDecimalFormatterOptions(); | |
out.groupingStrategy = args[0]; | |
out.someOtherConfig = args[1]; | |
return out; | |
}).apply( | |
null, | |
[ | |
terminusArgs[1], | |
terminusArgs[2] | |
] | |
) | |
(function () { | |
let out = new ICU4XFixedDecimalFormatterOptions(); | |
out.groupingStrategy = terminusArgs[1]; | |
out.someOtherConfig = terminusArgs[2]; | |
return out; | |
})() |
ICU4XFixedDecimal.new_.apply( | ||
null, | ||
[ | ||
terminusArgs[3] |
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.
Suggestion: If you have enough information to know that "it is the third argument", you probably also know the variable name.
Some(label) | ||
} | ||
}) | ||
.unwrap_or(param_name.clone()); |
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.
Suggestion: param_name.clone()
could instead use heck
to produce a nicer titlecase label
@@ -10,6 +10,7 @@ pub mod ffi { | |||
impl ICU4XLocale { | |||
/// Construct an [`ICU4XLocale`] from a locale identifier represented as a string. | |||
#[diplomat::attr(supports = constructors, constructor)] | |||
#[diplomat::demo(input(name(label = "Locale Name")))] | |||
pub fn new(name: &DiplomatStr) -> Box<ICU4XLocale> { |
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.
Suggestion: name
could be changed to locale_name
. This improvement could be applied elsewhere, too.
Rough implementation of the process described in https://docs.google.com/document/d/1xRTmK0YtOfuAe7ClN6kqDaHyv5HpdIRIYQW6Zc_KKFU/edit?usp=sharing
Proof of concept is live at https://ambiguous.name/diplomat/