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

Add option of existingType to .enum method to support repeated use of enums #2719

Merged
merged 3 commits into from Nov 9, 2018

Conversation

Projects
None yet
4 participants
@ekeric13
Copy link
Contributor

ekeric13 commented Jul 18, 2018

I was super happy to see knex supporting native enums in postgres, but when I had an enum that I used repeatedly across different columns in different tables, I was running into errors as the enum method created the type enum for you each time. The type would already exist so it would error. I only wanted to create the type once, afterwards I just wanted to use that enum.

Could we add a flag in the options param to not run the sql that creates the type enum?

The first time you run it you could do

table.enu('current_mood', ['sad', 'ok', 'happy'], { useNative: true, enumName: 'mood' })

And then the other times you can run

table.enu('mood', ['sad', 'ok', 'happy'], { useNative: true, enumName: 'mood', existingType: true })
@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Jul 18, 2018

I second this proposition and motion for a move to merge :P

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Aug 4, 2018

Sounds reasonable, I cannot think any better API for handling this situation.

@elhigu
Copy link
Collaborator

elhigu left a comment

Missing tests and documentation (specially intregration test will be important to see that this works the same way with other dialects supporting native enum)

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 6, 2018

@ekeric13 Would you consider finishing this PR?

@ekeric13

This comment has been minimized.

Copy link
Contributor

ekeric13 commented Nov 6, 2018

Yeah for sure. @heisian and I have a working implementation, just need to write tests for it.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 6, 2018

Awesome! Also PR for documentation in https://github.com/knex/documentation would be most appreciated :)

@ekeric13

This comment has been minimized.

Copy link
Contributor

ekeric13 commented Nov 6, 2018

Okay, @kibertoad added some tests.

Corresponding Docs: knex/documentation#147

Since it is just using an existing type, we could allow the api to be

table
	.enu('foo', undefined/null/irrelevant, {
		useNative: true,
		existingType: true,
		enumName: 'foo_type',
	})

As it is a bit weird to have to specify an array of values that aren't actually going to be used (since the type is already created, it just utilizes the enumName and not the array that is passed in).

The code would need to be changed to something like so:

  enu(allowed, options) {
    options = options || {};
	
	const values = options.useNative && options.existingType ? undefined : allowed.join("', '");

    if (options.useNative) {
      if (!options.existingType) {
        this.tableCompiler.unshiftQuery(
          `create type "${options.enumName}" as enum ('${values}')`
        );
      }

      return `"${options.enumName}"`;
    }
    return `text check (${this.formatter.wrap(this.args[0])} in ('${values}'))`;
  },

Finally it looks like we are still getting a drop in code coverage. I can add more tests, but they would be like "expect error when x" tests.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 6, 2018

@ekeric13 Thank you very much! It's 0:30 here already, it was a long day; I'll take a look at it tomorrow and comment ASAP, sorry for the delay.

@elhigu

elhigu approved these changes Nov 7, 2018

@ekeric13

This comment has been minimized.

Copy link
Contributor

ekeric13 commented Nov 8, 2018

So what is the process after this? Just will be merged in for the next release?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 8, 2018

@ekeric13 Probably can be merged within 0.16.0 window, but before that I assume we should consider the alternative improved syntax that you've suggested?..

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 8, 2018

@ekeric13 Looked at previous and current syntax and I think that it makes perfect sense, would you do the change?

@ekeric13

This comment has been minimized.

Copy link
Contributor

ekeric13 commented Nov 9, 2018

Okay, added the proposed ternary operator to make the function type agnostic of the values in the case where the values aren't being used.

Updated the docs as well for further clarification:
knex/documentation@e3d6f48

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 9, 2018

@ekeric13 Awesome work, thank you!

@kibertoad kibertoad merged commit 1c0dc2c into tgriesser:master Nov 9, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.7%) to 84.77%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 9, 2018

🎊

mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018

Add option of existingType to .enum method to support repeated use of…
… enums (tgriesser#2719)

* Update table column .enu to take an option that does not manually create the type

* Add tests for psql enum existingType

* Avoid utilizing enum values when using an existing type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment