search

Flowframe / laravel-trend

Generate trends for your models. Easily generate charts or reports.
MIT License
720 stars 74 forks source link

Add perWeek() interval #25

Closed marcoboers closed 1 year ago

marcoboers commented 1 year ago

This PR implements the perWeek() interval.

Related to #7

Larsklopstra commented 1 year ago

The behavior Postgresql returns isn't expected, according to the docs:

week of month (1–5) (the first week starts on the first day of the month)

This will result in duplicate keys

Larsklopstra commented 1 year ago

Will close this for now

jtomek commented 1 year ago

That's a pity.

ziming commented 1 year ago

@Larsklopstra is it fine to merge this but throw an exception if the site is using postgres driver?

This will allow us MySQL users to use it.

After all you did throw an error if user is not on mysql, pgsql or sqlite

mberneis commented 8 months ago

+1 here - I need a week interval as well

jtomek commented 8 months ago

I don't have the resources to dig into this right not, but Chatgepete suggested this:

― ChatGPT 4:

The issue arises with PostgreSQL's interpretation of the week. PostgreSQL's date format YYYY-W returns the week of the month, which can lead to duplicate keys because multiple months may have the same 'week number' (e.g., the first week of January and the first week of February would both be week 1).

To resolve this and ensure each week is uniquely identified, you might consider using a format that combines the year and the week of the year instead of the week of the month. PostgreSQL supports the IW format for the ISO week number and the IYYY format for the ISO week-numbering year. Using this combination ensures that each week is uniquely identified across years.

You could change the PostgreSQL format string in PgsqlAdapter.php from 'YYYY-W' to 'IYYY-IW'. This format respects the ISO-8601 definition of a week: starting on Monday and belonging to the year that contains the majority of its days.

―

What do you think?

danswiser commented 7 months ago

@Larsklopstra

I can't speak for others but I would expect ->perWeek() to use the calendar week (1-52) instead of the month week (1-5) anyway.

Unless there's a majority that are actually looking for the week number of a month (1-5) then it sounds like we'd be fine implementing the previous comment suggestions.

schmeits commented 4 months ago

@Larsklopstra if you change the formatter of the week to "IYYY-IW" for PgSQL it's working fine...

See the docs: IW | Week number of ISO 8601 week-numbering year (01-53; the first Thursday of the year is in week 1) IYYY | ISO 8601 week-numbering year (4 or more digits)

marcoboers commented 4 months ago

I added the PostgreSQL fix in https://github.com/Flowframe/laravel-trend/pull/63