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

Generate enums even with 1 value #783

Closed
stankovski opened this issue Mar 3, 2016 · 6 comments
Closed

Generate enums even with 1 value #783

stankovski opened this issue Mar 3, 2016 · 6 comments
Labels

Comments

@stankovski
Copy link
Member

Currently x-ms-enum tagged enums are generated as strings in C#

@amarzavery
Copy link
Contributor

@stankovski @emgerner-msft, @brjohnstmsft , @NiklasGustafsson, @markcowl

When should this happen?

  • If the single value enum is an optional model property or an optional parameter and if x-ms-enum extension is provided then it should be honoured.
    • modelAsString: true --> (No Validation happens) A Class with static properties in C#, Java. Node.js will expect the user to provide the underlying primary type on which the enum constraint was specified.
    • modelAsString: false --> An Enum in languages that support Enum. Validation in node.js against the allowed values.
  • If the single value enum is a required model property or a required parameter then it should always be treated as a constant. The x-ms-enum extension should be ignored.
    • Some clarifying questions
      • Would it make sense to generate a single value Enum if it is required (Server expects that specific value anyway)?
      • In the future when you add more values to the Enum, then it is abreaking change any way. So does the mechanism of breaking change ["Constant --> Enum with multiple value" OR "Single value Enum --> Multiple Value Enum" ] matter so much ?

@amarzavery
Copy link
Contributor

In addition, allowed values are rendered in the documentation comments for the property or parameter to provide some help to the user.

@amarzavery
Copy link
Contributor

#828

@amarzavery
Copy link
Contributor

closing this as the PR is merged.

@brjohnstmsft
Copy link
Member

brjohnstmsft commented Apr 25, 2016

@amarzavery Why can't this "enum-as-constant" behavior be opt-in? If I put modelAsString: false in my spec, AutoRest should always respect it when the target language supports enums. Instead, right now my model type that uses my single-valued enum now uses string instead of the enum type. In the following code, searchMode should be of an enum type:

        /// <summary>
        /// Initializes a new instance of the Suggester class.
        /// </summary>
        public Suggester(string name, string searchMode, IList<string> sourceFields)
        {
            Name = name;
            SearchMode = searchMode;
            SourceFields = sourceFields;
        }

        /// <summary>
        /// Gets or sets the name of the suggester.
        /// </summary>
        [JsonProperty(PropertyName = "name")]
        public string Name { get; set; }

        /// <summary>
        /// Gets or sets a value indicating the capabilities of the suggester.
        /// </summary>
        [JsonProperty(PropertyName = "searchMode")]
        public string SearchMode { get; set; }

In the future when you add more values to the Enum, then it is abreaking change any way. So does the mechanism of breaking change ["Constant --> Enum with multiple value" OR "Single value Enum --> Multiple Value Enum" ] matter so much ?

Yes, it does matter, because you're creating more work for customers to upgrade to the newer library.

I freely admit that it's lame having a single-valued enum, and it was premature generalization on our part when we designed the API. That said, we have shipped that API and a .NET client that models SuggestSearchMode as an enum, and it really sucks that this is now broken.

Please be careful when making such changes to AutoRest. They really need to be opt-in.

@brjohnstmsft
Copy link
Member

The problem with the enum type being modeled as a string was actually a bug that was fixed after 0.15.0, but my original point about being able to opt out of the "enum-as-constant" behavior remains.

jianghaolu added a commit to jianghaolu/AutoRest that referenced this issue Jun 16, 2016
6196ce0 Merge pull request Azure#20 from jianghaolu/master
06f30cf Checkstyle passes everywhere
2bf6b8b Merge pull request Azure#783 from jianghaolu/javadocs
b14cc72 Merge pull request Azure#779 from jianghaolu/paramhostfix
546e068 Network passes checkstyle
14b071c Merge commit '4aa3dd4b847e747387475e9249c4aba97b6ef8ac' into paramhostfix
1ae4191 Merge pull request Azure#19 from jianghaolu/updates
8b43962 Remove header after use
3e644d9 Fix parameterized host concurrency issue
95fa032 Done storage usages
7ad2207 Update dependencies to official releases

git-subtree-dir: ClientRuntimes/Java
git-subtree-split: 6196ce04c6741ceefc26f23f96c9e80a1dd78249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants