Skip to content

Conversation

clayellis
Copy link
Contributor

This PR changes the default postgreSQLEnumTypeName by adding the entire type's path to the name.

Example:

Consider the following case:

class Foo {
    var kind: Kind

    enum Kind: PostgreSQLEnum {
        ...
    }
}

class Bar {
    var kind: Kind

    enum Kind: PostgreSQLEnum {
        ...
    }
}

The two classes, Foo and Bar, each have a nested enum called Kind, each a PostgreSQLEnum.

Currently the default postgreSQLEnumTypeName for both of these nested enums is the same: "KIND". Meaning that the resulting Postgres ENUM type for the first would be overridden by the second with no indication to the user that this happened.

This PR would resolve that nuance by changing the default postgreSQLEnumTypeName to return unique names: "FOO_KIND" and "BAR_KIND".

Motivation:

This change is related to an issue discussed here. The motivating factor being, that in Swift, the types Foo.Kind and Bar.Kind are unique and distinguishable by the type system. It should then be reasonable to expect the default postgreSQLEnumTypeNames to be unique and the resulting Postgres ENUM types to be unique and distinguishable whereas currently, the first ENUM type created will be overridden by the second. This is counter-intuitive behavior, because it should be expected that two unique Swift enums will result in two unique Postgres ENUMs. In order to fully resolve the linked issue, and do so by default, this is a necessary change and will have a companion PR at Fluent to update the default migrationName (vapor/fluent#544).

Impact:

This could potentially have an impact on existing codebases by changing postgreSQLEnumTypeNames that are already in place.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

LGTM, tyvm!

// TODO: Determine if this should actually be uppercased.
// The PostgreSQL documentation for the ENUM type always
// shows this name being lowercased.
.uppercased()
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 it matches the other type names better if capitalized. We could discuss this further in an issue though.

@tanner0101 tanner0101 added the enhancement New feature or request label Jul 26, 2018
@tanner0101 tanner0101 self-assigned this Jul 26, 2018
@penny-coin
Copy link

Hey @clayellis, you just merged a pull request, have a coin!

You now have 15 coins.

@tanner0101 tanner0101 merged commit 9be6598 into vapor:master Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants