romeerez / orchid-orm

Orchid ORM
https://orchid-orm.netlify.app/
MIT License
488 stars 14 forks source link

[RFC] extend `t.timestamps()` #69

Closed bingtsingw closed 1 year ago

bingtsingw commented 1 year ago

I'd like to have the t.timestamps() to also support withoutTimeZone, add an extra timestampsWithoutTimeZone function is easy, but it seems we should add timestampsWithoutTimeZoneSnakeCase also, that's amazing api design.

So, I suggest to have only one timestamps() method which can accept optional parameters to configure it's behavior:

// pseudocode
function timestamps(config?: { timezone?: boolean; snakeCase?: boolean }) {
  let {timezone = true, snakeCase} =  config || {}

  if (typeof config.snakeCase === 'boolean') {
    // use config.snakeCase
  } else {
    // use this.snakeCaseKey
  }

  // build query depend on `timezone` and `snakeCase` 
}

Then we can remove t.timestampsSnakeCase()

bingtsingw commented 1 year ago

If you believe that there are no issues with this design, I can assist with its implementation as writing it doesn't seem too difficult.

romeerez commented 1 year ago

In fact, initially, it was without time zone, but then I read what postgres suggests and changed all timestamps to be with time zone by default.

Could you elaborate on why having timestamps without a timezone is important?

Postgres naming brings a lot of confusion, I guess it's by historical reasons, so you can have timestamp which doesn't store timezone, and timestamps with timezone which also doesn't store the timezone, but works slightly differently, and it's confusing.

romeerez commented 1 year ago

I think that having only timestamps() that are with timezone is enough and no need for other methods.

bingtsingw commented 1 year ago

I did see the suggests in the doc, but I'm used to saving all time as UTC in all of my systems, I already have a lot of projects running this way, I'm not going to change all tables to timestampTz for now, that would be tons of work.

bingtsingw commented 1 year ago

In fact, initially, it was without time zone, but then I read what postgres suggests and changed all timestamps to be with time zone by default.

Could you elaborate on why having timestamps without a timezone is important?

Postgres naming brings a lot of confusion, I guess it's by historical reasons, so you can have timestamp which doesn't store timezone, and timestamps with timezone which also doesn't store the timezone, but works slightly differently, and it's confusing.

That's interesting, I'm not very familiar with posgres's internal design, I'll do some research.

romeerez commented 1 year ago

Maybe let's rename existing timestampWithoutTimeZone to timestmapNoTz and then add timestampsNoTz and timestampsNoTzSnakeCase, how do you think?

Because if used extensively, I'm afraid that such timestampWithoutTimeZone will add much visual noise to the code.

bingtsingw commented 1 year ago

I tend to use method parameters because it allows for better combination and scalability. However, I think adding function implementations and explicitly using function names to distinguish functionality is also acceptable.

Anyway, NoTz is very nice.

romeerez commented 1 year ago

Ready: renamed it to timestampNoTZ, added timestampsNoTZ and timestampsNoTZSnakeCase methods for updatedAt and createdAt.