Skip to content
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

Schemas cleanup following model ADR #3 #8827

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

Rotorsoft
Copy link
Contributor

@Rotorsoft Rotorsoft commented Aug 12, 2024

Link to Issue

Closes: #8826

Description of Changes

  • Follows schema conventions in ADR

Test Plan

  • Basic testing since there are no major changes here, keeping an eye on date parsing

Copy link
Collaborator

@timolegros timolegros left a comment

Choose a reason for hiding this comment

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

I disagree with this #8824 (comment). Unless I am mistaken most if not all date types modified in this PR are directly related to Postgres TIMESTAMP columns. As such I think it may be beneficial to have 2 predefined date types with documentation:

// Use for any date that is required in the database but is automatically set by Sequelize or Postgres
`PG_REQUIRED_DATE = z.coerce.date().optional()`

// Use for any date that can be `null` in the database
`PG_OPTIONAL_DATE = z.coerce.date().nullish()`

Naming can of course be changed the idea is simply that we have 2 groups of dates and always remembering to use coerce and nullish or optional will get messy.

@Rotorsoft
Copy link
Contributor Author

I disagree with this #8824 (comment). Unless I am mistaken most if not all date types modified in this PR are directly related to Postgres TIMESTAMP columns. As such I think it may be beneficial to have 2 predefined date types with documentation:

// Use for any date that is required in the database but is automatically set by Sequelize or Postgres
`PG_REQUIRED_DATE = z.coerce.date().optional()`

// Use for any date that can be `null` in the database
`PG_OPTIONAL_DATE = z.coerce.date().nullish()`

Naming can of course be changed the idea is simply that we have 2 groups of dates and always remembering to use coerce and nullish or optional will get messy.

We can do this, but I'm questioning the value add of these utils...all dates should be coerced and the optional vs nullish decision is by column anyway... just trying to avoid unnecessary utilities, but if we decide to go with this, I would do a PG_DATE = z.coerce.date() only and leave the optional vs nullish to the schema

@timolegros
Copy link
Collaborator

We can do this, but I'm questioning the value add of these utils...all dates should be coerced and the optional vs nullish decision is by column anyway... just trying to avoid unnecessary utilities, but if we decide to go with this, I would do a PG_DATE = z.coerce.date() only and leave the optional vs nullish to the schema

Let's discuss with the others in office hours cause for me without adding nullish or optional to the utils then every time I add a date I need to figure out whether I need to use nullish or optional (and what each means) instead of just using the REQUIRED or OPTIONAL util which makes this decision easy.

@timolegros timolegros self-requested a review August 13, 2024 18:14
@Rotorsoft Rotorsoft merged commit 1bbcaee into master Aug 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema cleanup following ADR conventions in #8824
3 participants