-
Notifications
You must be signed in to change notification settings - Fork 0
Documentation for Enum support #129
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
Conversation
a6a5959 to
0f490c4
Compare
b7a8726 to
e5ccdbd
Compare
e5ccdbd to
8cebadd
Compare
| ```sql | ||
| select id from tasks where priority >= 'major'; | ||
| ``` | ||
|
|
||
| ``` | ||
| id | ||
| ---- | ||
| 1 | ||
| 3 | ||
| 4 | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to also list the priority in the result and maybe explain it, e.g.,
select id, priority from tasks where priority >= 'major';id | priority ---- 1 | major 3 | critical 4 | majorID 2 is filtered out because it has priority minor which was added before major and critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you're coming from.
What do you think of using a much simlper example, like
select 'major'::importance>'minor'::importance;instead? I feel like the slightly more complicated example currently presented in the doc is somewhat more intuitive, but you're right, that it is less self-explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both is fine. In general the one with the table is probably the more common use case, so I would go with this, but you can also do the other example or add both if you like.
5c9981a to
bd3dfba
Compare
| ### Change ownership | ||
| The owner of an enum can be changed via | ||
| ```sql | ||
| alter type enum_name owner to new_owner; | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure if we should maybe add some info about necessary permissions. Or should we only add this once the permissions are done? @ChrisWint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a previous draft of this page I explained the permissions as such:
This change can only be performed by the current owner or a superuser. The user requires permissions to SET ROLE to the new owner aswell as USAGE on the schema of the enum.
The new owner requires CREATE on the previously mentioned schema.
However, I removed this before opening the PR as I do not describe the required permissions for the other statements either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would wait for new permissions until the feature is live and then use a unified format in all statements to avoid having the same docs issues as postgres. I.e., docs for all statements and a Permissions section within all of these pages
bd3dfba to
c233db2
Compare
| [Create table as](createtableas) | ||
| : create and populate a new table with contents from a query | ||
|
|
||
| [Create type](/docs/references/datatypes/enums/#creation-of-enum-types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The create type statement is not only used for enums but we can also change this once we have implemented more of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking the same thing. Currently it makes sense, to simply refer to the enum page for this, but as more types become available, this would be highly confusing. I guess that's also why the postgres docu has its own entry for all the statements (for example create type), while the entry for Enums does not really touch on all of the statements acting on enums.
So at some point, we may have to push around the content a bit.
features documented: - create type - basic usage - comparison - drop type - alter type add value - alter type owner to adhered to Elena's comments
c233db2 to
745a322
Compare
With enum support, we need documentation. The docs are held in a very similar style to the postgres docu. Postgres scatters the documentation for some of the related functionality (e.g. drop type) a bit, which I am keeping more closely together.
Also, I am not sure to what extend we want to use running examples throughout the docu or not. I like them, but they arguably are more of a guide, than a documentation.