-
Notifications
You must be signed in to change notification settings - Fork 5
docs: improve README example #201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
c4870e9
docs: improve README example
embray 02ef7ee
docs: ensure proper asdf_value_destroy calls in README example
embray a87d743
test: fix the automake docs examples wrapper script to log stderr
embray bde46d2
docs: improve readme with simpler "Getting started" examples
embray d32b3d8
build: fix missing documentation files from source tarball
embray 16cfa07
docs: missing return statement in example
embray 8935dbd
docs: changes to example file to avoid certain confusions
embray 8392b1b
docs: use READE examples suggested by @braingram
embray 5757d66
docs: update example output in readme to reflect gh-204 fixes
embray daa836d
test: the readme examples must still support taking a filename argument
embray 14ca599
build: work around macOS endianness function declaration problems
embray File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ Usage | |
| usage/values | ||
| usage/writing | ||
| usage/extensions | ||
| usage/examples | ||
|
|
||
|
|
||
| API documentation | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitting the example into 2 programs (asdf-write and asdf-read) improves the approach-ability of the example(s) and makes them easier to follow.
However, this produces an invalid file (the serialized time is invalid).
The format also doesn't match roman files yet the example mentions Roman which is misleading. Roman files do not have any "obstime" or "observer" keys and the "data" is nested under a parent "roman" key.
Let's avoid linking libasdf to roman here in the README and cover that elsewhere (where roman files can be used).
I'd also say we avoid anything FITS-like here and cover that elsewhere.
How about an example file similar to the python example with a tree containing?:
(leaving out random since it seems overly complicated for this example)
The write could cover:
Read could cover accessing all of these values (and perhaps summing the sequence and squares).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not so sure about any of this to be honest.
I don't know how you got that. Might be using an older version of the library? The generated file has a copy committed in the repository (for comparison in the tests). It clearly outputs:
This is valid according to the schema (which I think itself is a bit buggy, but, the fact that this is accepted seems reasonable): https://github.com/asdf-format/asdf-standard/blob/main/resources/stable/schemas/stsci.edu/asdf/time/time-1.4.0.yaml
Though interestingly when I tried to open the file in Python I got a different exception -- it passed schema validation and threw:
This looks like a bug through and through. Makes no sense.
I could try changing the example to a different time format for now just to avoid any confusion from this, but will investigate the upstream bug...
This statement is also a bit confusing to me. It's just using Nancy Roman as an example "observer". I guess her name is on my mind for obvious reasons. I don't think anyone is ever confused in FITS examples when "OBSERVER = Edwin Hubble" that the example represents a valid HST data product.
But sure I could change it to Dennis Ritchie or Daffy Duck for all it matters.
I like the idea of exactly mirroring the Python example in principle. But unfortunately I don't think it translates as well to the C library in a succinct way. In Python, preparing a dictionary of small ndarrays is practically a one-liner. In C each one of those examples is several lines of setup and teardown code that has to be managed, bogging down the simplicity of the example.
The above statement is maybe also a good argument for adding some helper macros for creating simple ndarrays, or mapping builders. This is something I've been thinking about, but probably out of scope for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely yes. Pulling down fresh code and recompiling the library I don't see the odd 'iso_time'.
I wouldn't trust this schema to be a complete description of how time formats are handled. This is another reason to avoid using time in this example as that code has not been fully vetted.
Here's the candidate examples with some inline comments to help readers associate API with specific operations:
and reading:
I think it's helpful here to show:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fair enough, I'll use those.