Fix startOf function for isoWeek #23

stockiNail · 2020-08-05T16:28:42Z

Fixes #22

Fixes _isoWeekday function in order to calculate the right start of week

Uses the ABS of weekday in order that whatever number will be passed, the calculation will be always coherent

src/index.js

benmccann · 2020-08-05T19:05:44Z

This could probably use some tests

Changes the modulo instance as const instead of var. Adda blank before { for _isoWeekday function

The days diff defined as const instead of var

stockiNail · 2020-08-05T19:54:10Z

This could probably use some tests

Well, as you have seen, I'm not so expert on JS therefore I need time to understand how to do it.
Be also aware that I don't have the possibility to tests it on my laptop (probably I don't have the environment ready for that).
Locally, I've tested, of course.

Do you mean to add some tests into https://github.com/chartjs/chartjs-adapter-luxon/blob/master/test/specs/luxon-adapter.spec.js?

benmccann · 2020-08-05T19:59:14Z

Yeah, add a test in that file and then run gulp test-unit to see if it passes. (You'll have to run npm install once to install all the test dependencies but only once to set things up and not each time you run the tests)

I was thinking it'd be good to check each day of the week or at least the ones on the border like Sunday and Monday

Let me know if you have any other questions

benmccann · 2020-08-05T20:06:34Z

It looks to me like startOf('week') does the iso week in Luxon and there isn't a non-iso option. If that's the case then this could all be simplified to .startOf('week').plus({days: weekday - 1}) (that logic would be simple enough I'd be okay without adding a test, but you should probably manually verify it does what's expected)

stockiNail · 2020-08-05T20:17:50Z

It doesn't work with your suggestion.

If you try with 2020-08-01 (Saturday, 1st Aug), the startOf (passing 7, Sunday) should be Jul 26th but instead the result is Aug 2nd.
If you try my code, it works.

stockiNail · 2020-08-05T20:41:04Z

Yeah, add a test in that file and then run gulp test-unit to see if it passes. (You'll have to run npm install once to install all the test dependencies but only once to set things up and not each time you run the tests)

I was thinking it'd be good to check each day of the week or at least the ones on the border like Sunday and Monday

Let me know if you have any other questions

Thank you @benmccann

Let me take time but I'm gonna do. My doubt is to install something that I have to use only for this use case.... I don't usually use NodeJS and then npm as well.

benmccann · 2020-08-05T20:53:20Z

If you try with 2020-08-01 (Saturday, 1st Aug), the startOf (passing 7, Sunday) should be Jul 26th

Why is that? It doesn't sound right to me. The start of the week is July 27 (Monday). The 7th day of that week is Aug 2 (Sunday)

I just tried moment("2020-08-01").isoWeekday(7); and got Aug 2: https://plnkr.co/edit/i6q8LnRQP4oJHWqP?open=lib%2Fscript.js

stockiNail · 2020-08-05T21:04:24Z

Maybe I'm wrong to understand what weekday is.
For me weekday is the day that I want to be the first day of my week, overriding the 1 (Monday) usually used.
If it does not have this meaning, I think the weekday is quite useless and .startOf('week') then should be enough.
Again, maybe I'm wrong.

stockiNail · 2020-08-05T21:18:22Z

@benmccann I have created the unit test, checking all days of August and it works with gulp (not committed yet).

it('should startOf correctly using isoWeek preset', function() {
	const adapter = new _adapters._date();
	const dayOfWeekNames = ['Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat', 'Sun'];

	for (let dayOfWeek=0; dayOfWeek<dayOfWeekNames.length; dayOfWeek++) {
		for (let dayOfMonth=1; dayOfMonth<=31; dayOfMonth++){
			const dt = DateTime.local(2020, 8, dayOfMonth, 8, 30);
			const startOf = adapter.startOf(dt.valueOf(), 'isoWeek', dayOfWeek+1);
			expect(adapter.format(startOf, 'ccc')).toEqual(dayOfWeekNames[dayOfWeek]);
		}
	}
});

benmccann · 2020-08-05T21:22:56Z

Ah, no, weekday isn't the day that the week starts on. It's the day of the week to use with 1 being the first day of the week and 7 being the last day of the week

stockiNail · 2020-08-05T21:38:42Z

It's the day of the week to use with 1 being the first day of the week and 7 being the last day of the week

I'm completely lost, sorry. Let me try to do an example:

startOf ("2020-08-01", "isoWeek", 7): the result should be 2020-08-02 because I'm requesting the last day of the week of that date. Is it right?

Edit: if it is so, .startOf('week').plus({days: weekday - 1}) is correct.

kurkle · 2020-08-05T21:42:10Z

Date-fns adapter (and I think moment too) take the parameter as the start day of week.
So I think stokiNail is correct.

benmccann · 2020-08-05T21:47:17Z

The moment adapter does time.isoWeekday(weekday).valueOf()
https://github.com/chartjs/chartjs-adapter-moment/blob/e86d579262bc8a9e50f01ad85c89e95dff7b81e3/src/index.js#L47

And moment("2020-08-01").isoWeekday(7); returns Aug 2. Maybe I don't have the right terminology, but I'd expect that to be the result here too, unless I'm missing something?

stockiNail · 2020-08-05T21:55:50Z

@benmccann if you use .startOf('week').plus({days: weekday - 1}) by Luxon, you have got the same Moment's result: Aug 2.

If I understood correctly, startOf ("2020-08-01", "isoWeek", 7): the result should be 2020-08-02 because I'm requesting the 7th day of the week of that date.

I'm just wondering why the method is called startOf because in my opinion it is not returning any start of.
I can change the PR accordingly.

kurkle · 2020-08-05T21:56:46Z

Its date-fns adapter is doing a different thing then.
Its the start of (iso) week that confuses, I'd expect it to be first day of week always. The week starts on different day depending on location, so that's what I'd expect to be configurable.

stockiNail · 2020-08-05T22:13:07Z

@kurkle you're right. DateFns is doing what I thought, returns the first of the week and you can set the starting point.

Now I don't know anymore which is right and which is not.

If the startOf iso week should return the first day of the week (selected by the time's argument) based on weekday passed as argument, the Luxon (and maybe Moment) should be changed.

startOf ("2020-08-01", "isoWeek", 7) = "2020-07-26"

Instead, If the startOf iso week should return the day of the week (selected by the time's argument) based on weekday passed as argument, the Datefns should be changed.

startOf ("2020-08-01", "isoWeek", 7) = "2020-08-02"

Maybe I see another point of attention. For Datefns, the weekday must be between 0 (Sunday) and 6. For Luxon and Moment the weekday must be between 1 and 7 (Sunday). I don't see any normalization in the adapters.

benmccann · 2020-08-05T22:46:41Z

Hmm. The docs say that the property is a boolean and not a number, so now we have three ways its being done 😄

So I guess the question should be how do we want this to work and then we should update everything to match. Should we target the date-fns adapter's behavior and update the rest to match it?

stockiNail · 2020-08-06T06:28:44Z

@benmccann you have raised an interesting topic that I haven't checked yet.

The doc says

If true and the unit is set to 'week', then the first day of the week will be Monday. Otherwise, it will be Sunday.

that means startOf should return a Sunday or a Monday based on boolean value of isoWeekday property.
This sounds to be the use case of startOf therefore to stay simple, without changing too much, could be:

change the startOf adapters in order to return Monday of the week of the passed timestamp if weekday (its modulo by 7) is 1 otherwise Sunday for all the rest of the cases.

Make sense?

kurkle · 2020-08-06T06:58:44Z

I might be missing something. But ISO week starts from monday by definition and does not have partial weeks. I'm not sure if there is a week numbering system starting from sunday and having no partial weeks. And if there is, are the rules for first week the same (eg. 4th of jan is always on week 1)

US numbering has partial weeks. so Jan 1 is always first day of week, right? But that is the 'week' and not 'isoWeek'.

There are couple of other system in the US too, at least for broadcasting and accounting. The accounting one might be the one that isoWeek starting from sunday is used, but I'm not sure if that works in any/all of the date libs. (Because 1st of Jan is on the first week always in that one)

Middle Eastern system looks like it could be handled by 'week' numbering, but starting from saturday. So should probably have the parameter for 'week' unit too.

stockiNail · 2020-08-06T10:05:58Z

@kurkle you're right, and it's always an annoying topic.

Nevertheless, due to what the doc is saying clearly, you can start providing Monday or Sunday and maybe the first implementation (we are fixing the missing function on Luxon) could fit the current use case.

And then, you can start thinking which use cases could be addressed to improve it.

stockiNail · 2020-08-07T09:52:22Z

@kurkle @benmccann I had a look to CHART.JS code where and how isoWeek and startOf are used together.

There are 2 calls into https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.time.js

const {parser, round, isoWeekday} = options; // <-- here is reading the options and isoWeekDay is BOOLEAN
...
if (round) {
	value = round === 'week' && isoWeekday
		? scale._adapter.startOf(value, 'isoWeek', isoWeekday) // <-- here isoWeekDay is BOOLEAN, not NUMBER
		: scale._adapter.startOf(value, round);
}

and

const weekday = minor === 'week' ? timeOpts.isoWeekday : false; // <-- here is assigned as BOOELAN
....  
// For 'week' unit, handle the first day of week option
if (weekday) {
	first = +adapter.startOf(first, 'isoWeek', weekday); // <-- passed as BOOELAN
}

In both cases, the weekday argument of startOf is always a boolean (or 0-1).
I would suggest to change all adapters accordingly with this use case.

Furthermore I'd suggest to change the date adapter doc (in Chart.js) in the code changing the type of argument of startOf.
Last but not least, to change the new section for "definition types" applying the new argument type (boolean).

If you agree, I can try (taking time, sorry) to submit the issue and then the PR on all involved projects, starting from this one (luxon-adapter).

Let me know.

stockiNail · 2020-08-07T10:28:57Z

@benmccann @kurkle updated the PR accordingly with the previous post.

I have also added test cases for startOf isoWeek.

benmccann · 2020-08-07T13:18:21Z

src/index.js

@@ -23,7 +23,7 @@ Chart._adapters._date.override({
 	_create: function(time) {
 		return DateTime.fromMillis(time, this.options);
 	},
-
+	


should remove this newly added whitespace

benmccann · 2020-08-07T13:20:46Z

Yeah, you're right that it's treating isoWeekday as a boolean because if you passed 0 it would not do what you expected. What do you think @kurkle. Would you be okay with changing the adapters to a boolean?

stockiNail · 2020-08-07T13:29:13Z

@benmccann Just to complete the analysis.

The moment adapter is working if you use isoWeekday: true (looking for Monday) but doesn't work well looking for Sundays.

The date-fns adapter is working well because the weekday start from 0 (Sunday and it's also the numeric representation of false) and 1 is Monday( it's also the numeric representation of true). Let's say luckly :)

stockiNail · 2020-08-07T15:11:05Z

Would you be okay with changing the adapters to a boolean?

FYI I have already changed and tested the other adapters on my forks, as well, therefore I can push whenever you want.

kurkle · 2020-08-07T18:53:18Z

I'd prefer the 0-6 or 1-7, because I have an wip app supporting saturday as the first day of week (and using chart.js with date-fns)

benmccann · 2020-08-07T18:58:09Z

Ok, that sounds fine to me. It is more flexible.

Any preference on 0-6 or 1-7? I don't really have one. The date-fns one already uses 0-6, so that'd be fewer adapters to update 🙂 And it's also more standard programmer's indexing, so I guess I have some slight preference for it

stockiNail · 2020-08-07T19:33:57Z

Already changed using the week day between 0 (Sunday) and 6 (Saturday).
Added also test with true/false (as it is used today by Chart.js) and it works.

stockiNail · 2020-08-10T06:42:18Z

@kurkle @benmccann I have created 2 issues into moment and date-fns adapter project and the related PRs with the same logic.

kurkle · 2020-08-10T06:57:38Z

I'm wondering if the trunc/min/max is a bit excessive? It would actually hide an configuration or programming error. A slight performance hit maybe too.

stockiNail · 2020-08-10T07:03:00Z

I coded like that to avoid mistakes on date calculation.
The other option is to throw a RangeError (even if the trunc sounds to be needed as well).
In terms of performance, I have some doubts that those statements can decrease the performances too much.
Nevertheless I can implement as you want.

* Update rollup and add ESM output (#17) Update rollup and add ESM output * Update README with latest version (#15) * Remove bower (#19) * Remove use of helpers for v3 compatibility (#20) * Fix startOf function for isoWeek (#23) Fix startOf function for isoWeek * Bump version to 0.2.2 (#21) Co-authored-by: Colin Griffin <[email protected]> Co-authored-by: stockiNail <[email protected]>

Fix startOf function for isoWeek

stockiNail added 3 commits August 5, 2020 18:28

Fix startOf function for isoWeek

f1aacbc

Fixes #22

Fixes _isoWeekday calculation

efe4d6f

Fixes _isoWeekday function in order to calculate the right start of week

Normalized weekday argument

dd9da89

Uses the ABS of weekday in order that whatever number will be passed, the calculation will be always coherent

benmccann reviewed Aug 5, 2020

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

benmccann reviewed Aug 5, 2020

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

stockiNail added 2 commits August 5, 2020 21:46

Modulo as const

37e3f3e

Changes the modulo instance as const instead of var. Adda blank before { for _isoWeekday function

Uses const instead of var

26e49fe

The days diff defined as const instead of var

benmccann mentioned this pull request Aug 5, 2020

Bump version to 0.2.2 #21

Merged

uses weekday as boolean to get Sunday or Monday startOf

a043d81

benmccann reviewed Aug 7, 2020

View reviewed changes

remove whitespace

7d64f88

improved test case with a dymanic date

b3dde83

stockiNail added 2 commits August 7, 2020 21:25

weekday param as number between 0 (Sunday) and 6 (Saturday)

a34d4d9

added test with weekday passed as boolean

b29d708

stockiNail added 2 commits August 7, 2020 23:02

removes checking on range of weekday using trunc/max/min

16414f2

removed whitespaces from test-unit

bbae04a

kurkle approved these changes Aug 10, 2020

View reviewed changes

This was referenced Aug 10, 2020

Fix startOf function for isoWeek chartjs/chartjs-adapter-moment#17

Closed

Fix startOf function for isoWeek chartjs/chartjs-adapter-date-fns#22

Closed

etimberg approved these changes Aug 10, 2020

View reviewed changes

etimberg merged commit fca6f72 into chartjs:master Aug 10, 2020

etimberg pushed a commit that referenced this pull request Aug 21, 2020

Fix startOf function for isoWeek (#23)

5f164b5

Fix startOf function for isoWeek

stockiNail mentioned this pull request Sep 2, 2020

"time.isoWeekday" options on time scale as number chartjs/Chart.js#7749

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix startOf function for isoWeek #23

Fix startOf function for isoWeek #23

stockiNail commented Aug 5, 2020

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020

benmccann commented Aug 5, 2020

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020

stockiNail commented Aug 5, 2020

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020

stockiNail commented Aug 5, 2020 •

edited

Loading

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020 •

edited

Loading

kurkle commented Aug 5, 2020

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020

kurkle commented Aug 5, 2020

stockiNail commented Aug 5, 2020

benmccann commented Aug 5, 2020 •

edited

Loading

stockiNail commented Aug 6, 2020

kurkle commented Aug 6, 2020

stockiNail commented Aug 6, 2020

stockiNail commented Aug 7, 2020 •

edited

Loading

stockiNail commented Aug 7, 2020 •

edited

Loading

benmccann Aug 7, 2020

stockiNail Aug 7, 2020

benmccann commented Aug 7, 2020

stockiNail commented Aug 7, 2020

stockiNail commented Aug 7, 2020

kurkle commented Aug 7, 2020

benmccann commented Aug 7, 2020

stockiNail commented Aug 7, 2020 •

edited

Loading

stockiNail commented Aug 10, 2020

kurkle commented Aug 10, 2020

stockiNail commented Aug 10, 2020

Fix startOf function for isoWeek #23

Fix startOf function for isoWeek #23

Conversation

stockiNail commented Aug 5, 2020

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020

benmccann commented Aug 5, 2020

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020

stockiNail commented Aug 5, 2020

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020

stockiNail commented Aug 5, 2020 • edited Loading

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020 • edited Loading

kurkle commented Aug 5, 2020

benmccann commented Aug 5, 2020

stockiNail commented Aug 5, 2020

kurkle commented Aug 5, 2020

stockiNail commented Aug 5, 2020

benmccann commented Aug 5, 2020 • edited Loading

stockiNail commented Aug 6, 2020

kurkle commented Aug 6, 2020

stockiNail commented Aug 6, 2020

stockiNail commented Aug 7, 2020 • edited Loading

stockiNail commented Aug 7, 2020 • edited Loading

benmccann Aug 7, 2020

Choose a reason for hiding this comment

stockiNail Aug 7, 2020

Choose a reason for hiding this comment

benmccann commented Aug 7, 2020

stockiNail commented Aug 7, 2020

stockiNail commented Aug 7, 2020

kurkle commented Aug 7, 2020

benmccann commented Aug 7, 2020

stockiNail commented Aug 7, 2020 • edited Loading

stockiNail commented Aug 10, 2020

kurkle commented Aug 10, 2020

stockiNail commented Aug 10, 2020

stockiNail commented Aug 5, 2020 •

edited

Loading

stockiNail commented Aug 5, 2020 •

edited

Loading

benmccann commented Aug 5, 2020 •

edited

Loading

stockiNail commented Aug 7, 2020 •

edited

Loading

stockiNail commented Aug 7, 2020 •

edited

Loading

stockiNail commented Aug 7, 2020 •

edited

Loading