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

x-ms-discriminator-value #13

Merged
merged 34 commits into from
Sep 13, 2017
Merged

x-ms-discriminator-value #13

merged 34 commits into from
Sep 13, 2017

Conversation

mcardosos
Copy link
Contributor

Closing #4 and opening this
PTAL @jhendrixMSFT @marstr

@msftclas
Copy link

msftclas commented Sep 1, 2017

@mcardosos,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@mcardosos mcardosos changed the title Poly x-ms-discriminator-value Sep 1, 2017
@olydis
Copy link
Contributor

olydis commented Sep 11, 2017

@mcardosos I cancelled win32-x64 for some maintenance, consider this passed 😄

@olydis
Copy link
Contributor

olydis commented Sep 11, 2017

@mcardosos great, and commenting makes the job think a new commit was done, so now Linux reruns... feel free to merge X'D

@mcardosos
Copy link
Contributor Author

Copy link
Member

@marstr marstr left a comment

Choose a reason for hiding this comment

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

Mostly superficial stuff, I have a hard time commenting on a lot of the real meat of how we're changing the underlying model to accommodate the Polymorphic types.

@@ -13,10 +13,13 @@
type @Model.Name string

@EmptyLine
const (
@if (constants.Count() > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Changing this to constants.Any() will reduce this line from O(n) to O(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -113,6 +113,11 @@ public static string[] ToWords(this string value)
return WordSplitPattern.Split(value).Where(s => !string.IsNullOrEmpty(s)).ToArray();
}

public static string ToIdiomaticShortName(this string value)
{
return CodeNamerGo.Instance.GetVariableName(string.Concat(value.ToPhrase().Split(new char[]{' '}).Select(w => w[0])));
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be rewritten to be a little bit easier to read. Maybe,

public static string ToIdiomaticShortName(this string longName)
{
    var initials = from word in longName.ToPhrase().Split(new []{' '})
                         select word[0];
    var acronym = string.Concat(initials);
    return CodeNamerGo.Instance.GetVariableName(acronym);
}

Copy link
Member

@jhendrixMSFT jhendrixMSFT Sep 13, 2017

Choose a reason for hiding this comment

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

To throw another option into the mix, I wrote the follow function for creating vars from pascal-cased strings.

public static string ToShortVarName(this string name)
{
    var sb = new StringBuilder();
    for (int i = 0; i < name.Length; ++i)
    {
        if (char.IsUpper(name[i]))
        {
            sb.Append(char.ToLowerInvariant(name[i]));
        }
    }
    return sb.ToString();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}
}

public bool HasPolymorphicFields => AllProperties.Any(p => (p.ModelType is CompositeType && (p.ModelType as CompositeTypeGo).IsPolymorphic)
Copy link
Member

Choose a reason for hiding this comment

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

Too much shortening! Lets give this a real method body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made all this more readable

{
if (!string.IsNullOrEmpty(PolymorphicDiscriminator) && Properties.All(p => p.SerializedName != PolymorphicDiscriminator))
{
var newProp = base.Add(New<Property>(new
Copy link
Member

Choose a reason for hiding this comment

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

Why are we saving the result of base.Add? It doesn't look like it gets used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -113,6 +113,11 @@ public static string[] ToWords(this string value)
return WordSplitPattern.Split(value).Where(s => !string.IsNullOrEmpty(s)).ToArray();
}

public static string ToIdiomaticShortName(this string value)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't all names be Idiomatic? Maybe a more appropriate name would be ToShortName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -355,8 +360,23 @@ public static void AddImports(this IModelType type, HashSet<string> imports)
}

public static bool ShouldBeSyntheticType(this IModelType type)
{
return (type is PrimaryType || type is SequenceType || type is DictionaryType || type is EnumType ||
(type is CompositeTypeGo && (type as CompositeTypeGo).IsPolymorphicResponse()));
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to generate a synthetic type for polymorphic responses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they need a wrapper to be unmarshalled

@mcardosos mcardosos merged commit 61add46 into Azure:master Sep 13, 2017
@mcardosos mcardosos deleted the poly branch September 13, 2017 22:42
antkmsft pushed a commit that referenced this pull request Sep 16, 2022
* works for storage blob serivce now

* add TODO and some comments

* use === instead of ==

* use ternary operation

* use interface

* f

* javascript prettier

* Add todo
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.

5 participants