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

[TCGC][Bug] getAllModels, Optional literal do not have correct generated name #553

Closed
Tracked by #4300 ...
haolingdong-msft opened this issue Apr 1, 2024 · 7 comments · Fixed by #766
Closed
Tracked by #4300 ...
Assignees
Labels
lib:tcgc Issues for @azure-tools/typespec-client-generator-core library

Comments

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Apr 1, 2024

TSP definition:

model Model {
  literal: "literal";
  optionalLiteral?: "optionalLiteral";
}

Java code:
In Java's generated code, we will generate an enum for the optionalLiteral, the enum is called ModelOptionalLiteral, like below. I guess this optional literal is like anonymous model, and will need TCGC to generate a correct name for it.

public enum ModelOptionalLiteral {
    /**
     * Enum value optionalLiteral.
     */
    OPTIONAL_LITERAL("optionalLiteral");

    /**
     * The actual serialized value for a ModelOptionalLiteral instance.
     */
    private final String value;

    ModelOptionalLiteral(String value) {
        this.value = value;
    }

    /**
     * Parses a serialized value to a ModelOptionalLiteral instance.
     * 
     * @param value the serialized value to parse.
     * @return the parsed ModelOptionalLiteral object, or null if unable to parse.
     */
    public static ModelOptionalLiteral fromString(String value) {
        if (value == null) {
            return null;
        }
        ModelOptionalLiteral[] items = ModelOptionalLiteral.values();
        for (ModelOptionalLiteral item : items) {
            if (item.toString().equalsIgnoreCase(value)) {
                return item;
            }
        }
        return null;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public String toString() {
        return this.value;
    }
}

Discussed with @tadelesh offline, TCGC does not handle the name for it currently. It is better if TCGC can handle it.
@archerzz @pshao25 Does .Net have this behavior? What will .Net generate for optional literal?

@haolingdong-msft haolingdong-msft added lib:tcgc Issues for @azure-tools/typespec-client-generator-core library and removed needs-area labels Apr 1, 2024
@haolingdong-msft haolingdong-msft changed the title [TCGC][Bug] Optional literal do not have correct generated name [TCGC][Bug] getAllModels, Optional literal do not have correct generated name Apr 1, 2024
@tadelesh tadelesh self-assigned this Apr 1, 2024
@iscai-msft
Copy link
Contributor

i think this is something we can add. The root of the difference is java and .net want constants returned as single-value enums, while js and python will keep it as a single value constant.

We should have an input to tcgc that allows you to toggle this behavior. If an enum is returned in this case, we will automatically return a generated name ModelOptionalLiteral as well. To confirm, do .net and java want this enum to be fixed?

@haolingdong-msft
Copy link
Member Author

Hey @iscai-msft , thanks for the confirmation, for Java, it is fixed enum.

@tadelesh
Copy link
Member

tadelesh commented Apr 2, 2024

@pshao25 could you help confirm .net behavior?

@pshao25
Copy link
Member

pshao25 commented Apr 22, 2024

We are extensible enum currently, but it is very subject to change. So we don't have any request for TCGC for now.

@tadelesh
Copy link
Member

@haolingdong-msft i want to confirm java do this for all the constants or just model properties?

@haolingdong-msft
Copy link
Member Author

haolingdong-msft commented Apr 30, 2024

This is for all optional literal cases. Is there any difference between generating name for model property and for all cases from TCGC side?

@iscai-msft
Copy link
Contributor

the difference is right now, the constant type doesn't have a name property on it.

It makes sense to add this behavior in tcgc though, where we add .name and .isGenerated onto constants. This way languages (like .net and java) who would like to generate an enum / object for constants have a name to go off of

For now, I would say this isn't a blocker for tcgc adoption. You can keep your current code that comes up with a name for these constants (worst case you need to look into SdkConstantType.__raw to build the name for that constant).

@iscai-msft iscai-msft assigned iscai-msft and unassigned tadelesh Apr 30, 2024
github-merge-queue bot pushed a commit that referenced this issue May 8, 2024
fixes #553

---------

Co-authored-by: iscai-msft <[email protected]>
tadelesh pushed a commit that referenced this issue May 10, 2024
fixes #553

---------

Co-authored-by: iscai-msft <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib:tcgc Issues for @azure-tools/typespec-client-generator-core library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants