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

Fix issue with default string values being "\"\"" #389

Merged
merged 2 commits into from
Aug 29, 2022

Conversation

wdziemia
Copy link
Contributor

@wdziemia wdziemia commented Apr 27, 2022

Currently, string-optionals will default to quotes wrapped in quotes:

public static class Builder {

    protected String linkUrl;
    protected String optionalRlkey;
    protected String optionalPassword;

    protected Builder() {
        this.linkUrl = "\"\"";
        this.optionalRlkey = "\"\"";
        this.optionalPassword = "\"\"";
    }
  
}

Line 1053 takes in an empty value and returns \"\" to java_value, which already contains quotes around the string, causing optional values to pass a value of \"\" for optional parameters, potentially causing upstream issues.

Removing the default_value and passing field.default through to java_value resolves this issue and generates an empty string.

public static class Builder {

    protected String linkUrl;
    protected String optionalRlkey;
    protected String optionalPassword;

    protected Builder() {
        this.linkUrl = "";
        this.optionalRlkey = "";
        this.optionalPassword = "";
    }

}

d237d5c - This add the problematic line which returns empty quotes if the field default is empty.

#169 - This PR added a String format line which introduced the bug which formats quotes in additional quotes.

With a fix added, we see the generated code now properly generates an emtpy String value in EchoArg and EchoResult.

Screen Shot 2022-08-29 at 11 52 54 AM

@wdziemia wdziemia added the bug label Apr 27, 2022
@wdziemia wdziemia requested review from lchen8 and changusmc April 27, 2022 18:35
@wdziemia
Copy link
Contributor Author

Closing for now, will be picked up in a future PR.

@wdziemia wdziemia closed this Apr 27, 2022
@handstandsam handstandsam deleted the fix_optional_params branch July 11, 2022 21:55
@wdziemia wdziemia restored the fix_optional_params branch August 24, 2022 19:19
@wdziemia wdziemia reopened this Aug 24, 2022
@wdziemia wdziemia force-pushed the fix_optional_params branch from 71d1438 to 295a9dc Compare August 29, 2022 15:33
@wdziemia wdziemia force-pushed the fix_optional_params branch from 295a9dc to c97a387 Compare August 29, 2022 17:29
@handstandsam
Copy link
Contributor

Paired with @wdziemia on related work #418, can approve this once this PR is rebased and we see the changes.

Copy link
Contributor

@handstandsam handstandsam left a comment

Choose a reason for hiding this comment

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

The check worked!!!

Super easy to see that your changes to the Stone Python Generator works as intended!

@wdziemia wdziemia merged commit 8b4ca7d into master Aug 29, 2022
@handstandsam handstandsam deleted the fix_optional_params branch September 22, 2022 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants