ENH: Validate optional fractional seconds in AcquisitionTime in scans.tsv

[sappelhoff]

closes #957

code climate does not like my changes ... but they work. :-)

suggestions for improvements are welcome. cc @tsalo @rwblair

[adam2392]

A good clean simple improvement. Looks pretty good to me!

[effigies]

Is there a reason that we're creating our own format string, rather than just using parseISO?

> var parseISO = require('date-fns/parseISO')
> parseISO('2017-05-03T06:45:45Z')
2017-05-03T06:45:45.000Z
> parseISO('2017-05-03T06:45:45.2Z')
2017-05-03T06:45:45.200Z
> parseISO('2017-05-03T06:45:45.22Z')
2017-05-03T06:45:45.220Z
> parseISO('2017-05-03T06:45:45.222Z')
2017-05-03T06:45:45.222Z
> parseISO('2017-05-03T06:45:45.2222Z')
2017-05-03T06:45:45.222Z
> parseISO('2017-05-03T06:45:45.22222Z')
2017-05-03T06:45:45.222Z

Are there values it accepts that we don't want to?

[effigies]

Oh, I see. RFC3339 is a strict subset of ISO8601, and we're doing almost RFC3339. I think we can do this by checking that it's valid ISO8601 and then that it matches our format of RFC3339 - offset.:

const rfc3339ish = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(.\d+)?$/
var isValid = dateIsValid(parseISO(acq_time)) && rfc3339ish.test(acq_time)

Feels cleaner than a loop, IMO.

[sappelhoff]

Thanks for your suggestion, I'll give it a try soon!

RFC3339 is a strict subset of ISO8601, and we're doing almost RFC3339

@effigies could you clarify what you mean by "almost RFC3339"? To my knowledge, we are following RFC3339 (at least that was the plan 🤔 )

If there is a deviation, we should consider fixing it (most recent changes with frac secs are not yet released), because IMO it's not a good idea to almost follow a standard (or in this case, subset of ISO).

• https://bids-specification.readthedocs.io/en/latest/02-common-principles.html#units

One (potentially not-fixable-because-breaking) divergence that I can see is with regards to the timezone. Let me quote from the BIDS spec:

YYYY-MM-DDThh:mm:ss[.000000] This is equivalent to the RFC3339 "date-time" format, time zone is always assumed as local time).

apart from the superfluous ), RFC3339 actually assumes UTC and different timezones are supplied with an offset ... whereas in BIDS we seem to assume "local time" --> and we have no way of knowing this local time in a dataset, because there is no BIDS field capturing the recording site on the earth.

---

PS: for (my own) later reference, this is a nice article: https://medium.com/easyread/understanding-about-rfc-3339-for-datetime-formatting-in-software-engineering-940aa5d5f68a

[effigies]

Yeah, the "almost" refers to the fact that, in RFC3339, the time zone is required. It can be either Z or an explicit offset. We relax that and permit people to use local time. I think that's fine, as relative times is all a compliant BIDS app (accepting anonymized datasets) can rely on being valid, so it doesn't really matter.

[rwblair]

Just the 'var'->'let' update. Other wise looks good to me.

[rwblair]

Use let here instead of var if possible.

[rwblair]

A note if we ever run into performance concerns, parse with format "yyyy-MM-dd'T'HH:mm:ss.SSSSSSS" will work on a date with fractional seconds up to 7. It won't return the correct fraction but I believe its functionally equivalent for testing validity:

dateIsValid(parse("2017-05-03T06:45:45.9", format, new Date()))
true
parse("2017-05-03T06:45:45.9", format, new Date())
2017-05-03T11:45:45.000Z

[sappelhoff]

@rwblair are you recommending against @effigies's suggestion in https://github.com/bids-standard/bids-validator/pull/1019#issuecomment-664095375 ?

[rwblair]

@rwblair are you recommending against @effigies's suggestion in #1019 (comment) ?

@sappelhoff Sorry for the confusion, no I am not. Was referring only to the changes in my code review. His suggestion seems reasonable if it works just as well as the current implementaiton.

[sappelhoff]

The tests for TSV spec pass locally and @effigies solution is nice and elegant IMO.

A bunch of unrelated tests seem to fail however.