-
Notifications
You must be signed in to change notification settings - Fork 6
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 check constraints instead of varchar #55
add check constraints instead of varchar #55
Conversation
sjib
commented
Dec 12, 2023
•
edited
Loading
edited
- fixes remark fields are limited to 80 chars #10
- fixes tww_sys.dictionary_value_list should not be here as requested in fix re_building_group_disposal missing fk_columns #49 (comment)
- update data_media.path definitions
tww_sys.dictionary_value_list as requested in #49 (comment)
for more information, see https://pre-commit.ci
@@ -61,7 +61,8 @@ WITH ( | |||
CREATE SEQUENCE tww_od.seq_txt_symbol_oid INCREMENT 1 MINVALUE 0 MAXVALUE 999999 START 0; | |||
ALTER TABLE tww_od.txt_symbol ALTER COLUMN obj_id SET DEFAULT tww_sys.generate_oid('tww_od','txt_symbol'); | |||
COMMENT ON COLUMN tww_od.txt_symbol.obj_id IS 'INTERLIS STANDARD OID (with Postfix/Präfix), see www.interlis.ch'; | |||
ALTER TABLE tww_od.txt_symbol ADD COLUMN classname varchar(50) ; | |||
ALTER TABLE tww_od.txt_symbol ADD COLUMN classname text; | |||
ALTER TABLE tww_od.txt_symbol ADD CONSTRAINT length_max_50_symbol_classname CHECK(char_length(classname)<=50); |
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.
ALTER TABLE tww_od.txt_symbol ADD CONSTRAINT length_max_50_symbol_classname CHECK(char_length(classname)<=50); | |
ALTER TABLE tww_od.txt_symbol ADD CONSTRAINT classname_max_length CHECK(char_length(classname)<=50); |
I would avoid extra long names (constraints are defined for tables, so the table name is implicit) and put the column identifier in front
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 do not have a strong opinion on this except the constraint name should be :
- as understandable as possible
- unique (for views like tww_ws and tww_reaches)
- as small as possible
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 can use the shortcut of the class to still make it unique. Else we will have for each class an identifier_max_length
So better dt_identifier_max_length
(instead of drainless_toilett_identifier_max_lenght')
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.
@3nids New version pushed - please check
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.
They don't need to be unique as they are defined at the table level.
Again, I'd
- drop the prefix
- drop the length info (if the length changes, no need to rename the constraint)
- length_max => max_length
but it's not that big of a deal.
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.
From a users perspective it is helpful to understand why you get the error message. Let's go ahead as technically there are no obstacles anymore.
…ttps://github.com/teksi/wastewater into 2023-12-12-add-check-constraints-instead-of-varchar
for more information, see https://pre-commit.ci
@ponceta you wanted to review this? |
Merge upstream