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

[ekeric13]

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]

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

[elhigu]

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

[elhigu]

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]

@ekeric13 Would you consider finishing this PR?

[ekeric13]

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

[kibertoad]

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

[ekeric13]

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]

@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.

[ekeric13]

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

[kibertoad]

@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]

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

[ekeric13]

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]

@ekeric13 Awesome work, thank you!

[heisian]

🎊