Added write_key_cmt method to include comments with keys.
#307
Conversation
There was a problem hiding this comment.
Hi, thanks for raising this PR. I really appreciate it when people are using this crate!
I have a concern about the API. I would really not like to add too many methods to the public API.
Some options I considered:
- make the
write_keymethod take an optional comment
- the downside is the user will have to explicitly pass
Noneif they don't care about the comment - an upside is it might get people to think more meaningfully about writing fits comments, even though they are restricted in length which is a pain to use
- make the
write_keymethods use the builder pattern, where they return a builder object with a.write()method to actually perform the write, and can include awith_commentmethod to add a comment
- this could be quite verbose
- builder patterns are great for types with many fields, but there is only one extra field here really so it could be overkill
- keep your implementation
- adds almost duplicate methods to do nearly the same thing
- its what a C API design might look like, rather than a Rust API
- Make the write key methods generic over a trait that's implemented by a simple value, or a couple of value string pairs.
- ???
I am open to suggestions here. We are still pre-1.0 so we could always change our minds in the future. I am personally siding with option 4.
Also: this PR supports writing comments with keys, but it might be nice to be able to read comments as well. I don't mind this being a follow up PR, but I think we should add that functionality, once we have settled on a nice API.
|
The only issue I see with 1 or 2 is that it’s going to be API breaking, which is something I wanted to avoid. I do agree that my implementation is very C-like. I do like the idea of a generic trait, that’s more Rust-like. I will give this a bit more thought. I agree that we should be able to read the comments as well. Will you ever consider a pure Rust implementation of FITSIO? |
I will have a bit of an experiment as well and get back to you.
👍
I think there are a few already, e.g. https://github.com/cds-astro/fitsrs, https://docs.rs/fits-rs/latest/fits_rs/, https://github.com/malikolivier/fitrs. I also had the thought myself, as the file format has a reasonable spec, however never got particularly far with the implementation. |
|
I've drafted my new API design in #332, let me know what you think. Particularly the updates to the full example, which show how to use this new API. |
# 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 * [x] `write_key` support * [x] `read_key` support * [x] perform assertions on header card lengths * [x] rewrite the change in a backwards compatible way - [x] writing a header value is backwards compatible already - [x] make `read_key` take either a primitive value (backwards compatible) or `HeaderValue<_>` * [x] remove public access to `HeaderValue` internals? Or remove methods * decifded to keep fields as the type should be simple
|
Thanks for this contribution, but as discussed We are going with the implementation from #332 |
I have added a function
write_key_cmtto allow the addition of comments along with the key.