add units to offset/duration fields

[jrevels]

For now, this PR is a simple code readability improvement to make up for the fact that these fields aren't typed Second.

On the one hand, it makes sense for these quantities to be typed Float64, as EDF+ serializes some of these quantities as numeric strings in floating-point notation.

On the other hand, it looks like EDF.jl silently truncates non-integers to integers when writing. Assuming my reading of that is correct (it might not be!), wouldn't it be better to just type these fields as Second for now, so that users trying to encode non-integer durations will get a nice type error instead of unknowingly writing incorrect data? Then, once EDF.jl actually supports fractional durations/offsets, that front-end restriction can be lifted. If my reading is correct I can make that change to type the fields as Second, just let me know. EDIT: I am dummy

[codecov-io]

Codecov Report

Merging #11 into master will not change coverage. The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #11   +/-   ##
=======================================
  Coverage   98.94%   98.94%           
=======================================
  Files           3        3           
  Lines         189      189           
=======================================
  Hits          187      187           
  Misses          2        2

Impacted Files	Coverage Δ
src/EDF.jl	100% <100%> (ø)	:arrow_up:
src/read.jl	98.29% <100%> (ø)	:arrow_up:
src/write.jl	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 08cf659...79427db. Read the comment docs.

[ararslan]

it looks like EDF.jl silently truncates non-integers to integers when writing.

The condition there is isinteger(x) ? trunc(Int, x) : x, which means that e.g. 1.2 remains 1.2 and 1.0 becomes 1. It could have equivalently been written isinteger(x) ? Int(x) : x, as nothing is actually being truncated.

[ararslan]

The name duration_in_seconds is kind of annoyingly long, IMO. Could we just document that the units are seconds?

[jrevels]

The name duration_in_seconds is kind of annoyingly long, IMO. Could we just document that the units are seconds?

IMO the readability win is big enough to justify it (nice to be able to just see code and know the units without needing to jump to docs), but that's more my personal preference and it is kind of long 😛Can have this just be a doc PR if you'd prefer.

[ararslan]

Meh, it's fine. Though I suppose if we wanted to be kind citizens we'd need to deprecate accessing the old names, but I'm feeling pretty meh about that too.

[jrevels]

closing as stale

[ararslan]

Probably a good change to make at some point