Skip to content
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

c#: Add cli option to generate Result types #1115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsturtevant
Copy link
Collaborator

Adds option to generate Result types instead of handling Results via Exceptions: #1042

@alexcrichton alexcrichton requested a review from dicej January 8, 2025 15:44
@jsturtevant jsturtevant self-assigned this Jan 8, 2025
@jsturtevant jsturtevant requested review from silesmo and yowl January 8, 2025 16:39

/// Generates code for wit Result types instead of exceptions
#[cfg_attr(feature = "clap", arg(long))]
pub with_wit_results: bool,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open to name suggestions

Copy link
Collaborator

@dicej dicej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; thanks! Just a few whitespace and grammar nits.

@@ -30,6 +30,10 @@ pub struct Opts {
/// Skip generating `cabi_realloc`, `WasmImportLinkageAttribute`, and component type files
#[cfg_attr(feature = "clap", arg(long))]
pub skip_support_files: bool,

/// Generates code for wit Result types instead of exceptions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Generates code for wit Result types instead of exceptions
/// Generate code for WIT `Result` types instead of exceptions

var result = imports.test.results.TestInterop.EnumError(a);
if (result.IsOk) {
return Result<float, ITest.E>.Ok(result.AsOk);
}else{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}else{
} else {

uwrite!(
self.src,
"\
if ({previous}.IsOk) {{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Would you mind adjusting the indentation for this and the other uwrite expressions that follow? E.g. put the if under the " on the previous line or similar and indent the var on the next line four columns past that, etc. It's a bit jarring to see it indented way to the right.

Comment on lines +275 to +277
}} else {{
throw new {exception_name}({var_name}.AsErr!, {level});
}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}} else {{
throw new {exception_name}({var_name}.AsErr!, {level});
}}
}}
else
{{
throw new {exception_name}({var_name}.AsErr!, {level});
}}

uwrite!(
self.src,
"\
if ({previous}.IsOk) {{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the C# I think we are trying to go more the style of { on the next line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ({previous}.IsOk) {{
if ({previous}.IsOk)
{{

let tail = oks.iter().map(|_| ")").collect::<Vec<_>>().concat();
cases.push(format!(
"\
case {index}: {{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case {index}: {{
case {index}:
{{

payload_is_void = result.ok.is_none();
}
if !self.results.is_empty() {
self.src.push_str("try {\n");
Copy link
Collaborator

@yowl yowl Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.src.push_str("try {\n");
self.src.push_str(\"
try
{\n");

Not sure I've got the rust syntax right here.

let cases = cases.join("\n");
uwriteln!(
self.src,
r#"}} catch (WitException e) {{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
r#"}} catch (WitException e) {{
r#"}}
catch (WitException e)
{{

Copy link
Collaborator

@yowl yowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks,

uwriteln!(
self.src,
r#"}} catch (WitException e) {{
switch (e.NestingLevel) {{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
switch (e.NestingLevel) {{
switch (e.NestingLevel)
{{

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants