edtf.js

edtf.js copied to clipboard

Here is my first suggestion draft to tackle issue #20 . All previous tests pass without any issues so it does not seem to be too breaking.

What it does

I tried to limit the changes to only alter the code's behavior in the specific situations where it would have confusing results. (ie. to change the year value only when necessary to fit the EDTF pattern) This implied 2 changes

1st change: in the Bitmask element

• I created a lastLeapYear(year) instance method to the Bitmask object: given the object's value and its year parameter containing a given year (in the form of a positive Number), the method will return the biggest leap year that can match the combination of these 2. If there is no match or if the given year is already a leap year, the method will return the given year.

• In the max method, when the month and day are fully specified and correspond to February 29th, we make use of that lastLeapYear method above to replace the year that we will return with the biggest leap year instead. This change avoids erroneously moving the day back to 28 in the next line in the case of an unspecified year that matches leap years.

2nd change: in the Date element

Any time a new edtf.Date is created, after parsing and generating the desired UTC date like before:

1. We check if the year is unspecified but the month and day are specified
2. If not, fall back to the usual behavior. Else, we check (using our lastLeapYear created above) if any leap year matches the year and unspecified Bitmask that were given as arguments
3. If not, fall back to usual behavior. Else, we replace the year from the original args with the top leap year (or bottom leap year if BC) possible given by lastLeapYear, and create a new UTC date from these args.
4. If the date value from the newly created date differs from the one in the original date, it means the leap year matters in this, so we use this newly created date instead of the original one.

Possible issues with this draft fix

• Bitmask reuses a part of Date.pad to convert years to string in the lastLeapyear method. We should find a way to combine this bit. Unfortunately, the utils.js file depends from bitmask.js, so it couldn't fit there...

• Some of the conditions may feel a little complex, which may be due to a lack of understanding of the tooling in my disposition. Some of the code can probably be easily refactored to read much nicer than it currently does.

• Maybe this fix adds too much complexity? I stumbled a lot before coming to this acceptable answer, I could not see a better way to fix issue #20 while having the least impact on the rest of the library.

It is to be noted that the pad code can easily be turned into a slice one-liner:

`0000${abs(k)}`.slice(-4)

Not very elegant, but short!

Sorry about the late response! I've been out of town over the break, so have not had the time yet to look at this. At first glance this looks great, but I'd like to take some time to think about the issues you're raising above.

No worries, still here, glad to see you back 👍

Take your time and feel free to edit most of it, I am not convinced myself that this PR is production ready, but at least it begins to solve the issue.