Skip to content

Compare enums semi-structurally. #6036

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

Merged
merged 12 commits into from
Dec 18, 2015
Merged

Compare enums semi-structurally. #6036

merged 12 commits into from
Dec 18, 2015

Conversation

sandersn
Copy link
Member

Fixes #1748.

  1. Unqualified names must match.
  2. Target contains members with same names as all source members.

1. Unqualified names must match.
2. Target contains members with same names as all source members.
@weswigham
Copy link
Member

Could you add some tests with const enums and enums with computed values, just so they have some coverage? Otherwise seems great, 👍

const targetMembers = arrayToMap(targetDecl.members, member => getTextOfPropertyName(<PropertyName>member.name));
for (const member of sourceDecl.members) {
const name = getTextOfPropertyName(<PropertyName>member.name);
if (!targetMembers[name]) {
Copy link
Member

Choose a reason for hiding this comment

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

You need to use hasProperty here

Copy link
Member Author

Choose a reason for hiding this comment

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

done

target.symbol.flags & SymbolFlags.ConstEnum) {
return Ternary.False;
}
const sourceDecl = <EnumDeclaration>getDeclarationOfKind(source.symbol, SyntaxKind.EnumDeclaration);
Copy link
Contributor

Choose a reason for hiding this comment

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

what I meant is it that with this code

enum A { X }
enum A { Y }
let a = A.X | A.Y // A has both X and Y

you will see only X member, but you won't see Y since you are using one declaration to obtain property names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there already a right way to get all members of enums? Otherwise I'll need to write getDeclarationsOfKind and then look at all members from those symbols.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, you already have types on hand, so you can get its members that has flag SymbolFlag.EnumMember. Open question is if you need to pay attention to the order of member since it can directly affect values of enum members

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe in the discussion around #1748 last week, we decided to ignore values of members. So as long as the enums are not const, we will just check for existence of matching property names.

Copy link
Member Author

Choose a reason for hiding this comment

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

The type object for enums don't actually have members defined, so there are no members to check for EnumMember.

Copy link
Contributor

Choose a reason for hiding this comment

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

symbol for a enum type should have exports that contain all enum members

Copy link
Contributor

Choose a reason for hiding this comment

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

resolveStructuredTypeMembers(enumType).members should do the trick

Use getTypeOfSymbol >> resolveStructuredTypeMembers >> properties instead
of looking at declarations.
}
const sourceMembers = resolveStructuredTypeMembers(getTypeOfSymbol(source.symbol)).properties;
const targetMembers = resolveStructuredTypeMembers(getTypeOfSymbol(target.symbol)).properties;
const targetNames = arrayToMap(targetMembers, member => member.name);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, given what Wesley said, I think it was correct to do what you were doing before when creating the map. If member.name isn't astring, how is this working? Don't you need member.name.text?

Copy link
Member Author

Choose a reason for hiding this comment

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

member.name is a string:

    function Symbol(flags: SymbolFlags, name: string) {
        this.flags = flags;
        this.name = name;
        this.declarations = undefined;
    }

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's the symbol, not the member itself. This is fine then.

@sandersn
Copy link
Member Author

@vladima any more comments?

}
for (const property of getPropertiesOfType(getTypeOfSymbol(source.symbol))) {
if (property.flags & SymbolFlags.EnumMember) {
const targetProperty = getPropertyOfType(getTypeOfSymbol(target.symbol), property.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

lift it to a variable so it is not refetched on every iteration (cost of it is probably low but still)

@vladima
Copy link
Contributor

vladima commented Dec 17, 2015

👍 after minor nit from comment is addressed

sandersn added a commit that referenced this pull request Dec 18, 2015
@sandersn sandersn merged commit 4d792f2 into master Dec 18, 2015
@sandersn sandersn deleted the structural-enums branch December 18, 2015 00:31
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
@DanielRosenwasser
Copy link
Member

Keywords: enum identical names relate related to assignable compatible compatibility

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

Successfully merging this pull request may close these issues.

5 participants