feat: POC header comment API design

[simonrw]

Motivation

Inspired by #307, can we design a nice API to read/write comments of header cards?

Changes

• write_key takes either a simple value, or a tuple of (value, comment) for writing comments
• read_key returns a new structured type which contains both the value and comment
• tests and docs updated
• restructure the headers code
  • create new headers module with previous code
  • add constants module, mostly with header related lengths
  • add header_value module defining the HeaderValue<T> type along with its std trait implementations

TODO

• write_key support
• read_key support
• perform assertions on header card lengths
• rewrite the change in a backwards compatible way
  • writing a header value is backwards compatible already
  • make read_key take either a primitive value (backwards compatible) or HeaderValue<_>
• remove public access to HeaderValue internals? Or remove methods
  • decifded to keep fields as the type should be simple

[coveralls]

coverage: 76.094% (-0.7%) from 76.797%
when pulling 6c7fc37 on write-header-comments
into 1ebd973 on main.

[sunipkm]

I think this is the best way to have implemented this. The write APIs are unaffected, and read API only needs an additional identifier to differentiate between the value and the comment. Awesome!

As for deprecating the old method and adding a new one, read_key sounds more generic than (e.g.) read_fkey.
On the other hand, we could follow the same rationale as dropping xrange (which was better than range in Python 2.7) and making the behaviour of range in Python 3 the same as Python 2.7 xrange. However, range and xrange did not differ in calling convention, only in behavior. I guess this band-aid would need to be ripped off at some point, and the earlier would be better.

[simonrw]

@sunipkm thanks for the feedback.

I think this is the best way to have implemented this. The write APIs are unaffected, and read API only needs an additional identifier to differentiate between the value and the comment. Awesome!

Your comment made me think of a backwards-compatible way of implementing this! I now implement ReadsKey for both primitive values (e.g. i64) and HeaderValue<T>. The caller can decide what they want. If they just want the value, they can use the primitive version. If they need the comment, they can use the HeaderValue<T> version.

My concern is that this seems like a backwards-compatible change, but it might not be. I think it now hugely depends on the type inference used by the caller. If it is not ambiguous (e.g. they pass the header value to a function that accepts an i32) then we are golden. If they pass the value to something that prints it (i.e. uses the Debug impl) then their output will change.

As for deprecating the old method and adding a new one, read_key sounds more generic than (e.g.) read_fkey.
On the other hand, we could follow the same rationale as dropping xrange (which was better than range in Python 2.7) and making the behaviour of range in Python 3 the same as Python 2.7 xrange. However, range and xrange did not differ in calling convention, only in behavior. I guess this band-aid would need to be ripped off at some point, and the earlier would be better.

Are you sure you want to cite the Python 2 -> 3 transition?!?!?! (I remember it being painless for the most part, but I waited ages to upgrade, by which time the libraries I used had caught up.)

I appreciate the idea of moving fast and breaking things, which we can do as we are pre-1.0, however I value stability higher than API correctness.

@cjordan: as a user of this crate, does your current code compile if you check out this branch?

[simonrw]

Note: the coverage has gone down because we include some functionality for the HeaderValue type that is not tested in the --lib tests, but it is tested in the --doc tests.

[cjordan]

Hi @simonrw. I'm not able to check that this change will affect the code I used to work on (as of this year, I'm also an ex-astronomer), because those crates depend on fitsio version 0.20. However, the code looks good to me. I think it's a nice quality-of-life improvement.

[sunipkm]

@simonrw haha I did the same, waited till Py 2.7 went EOL. I guess Py 2 -> 3 transition isn’t the best example but the point I was trying to make is, we should encourage the assignment of the more flexible interface/implementation to the easier-to-read method name, because those stick. If we deprecate read_keys now with the old behavior, shouldn’t we technically remove that method in the future completely and not replace it with the new behavior method, since the calling conventions will be different?

I agree on the point that you bring up, that it looks backwards compatible but it isn’t. I guess as long as function calls are mostly fine, it should be alright for the end user. String conversion of a number and read-back of it may be error prone anyway, e.g. how 268 is ambiguous unless we know the number system used to write it — and this bit of inconsistency/inconvenience now will greatly benefit the users down the line.

[simonrw]

Thanks both for your comments. I think it is safe to release this with a minor (so pre-release major) bump. I think type inference should revert to the old read_key implementation, or the user would have to be explicit about the type anyway, so I think we are ok.