Skip to content
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

ZEP1: Apply review feedback for terminology section #163

Conversation

jstriebel
Copy link
Member

This PR applies some feedback from the ZEP1 review in #149. It adds illustrations to the terminology, and improves the store and storage transformer sections. In detail:

  • Added two illustrations, one for the general hierarchy and array terminology, and one for the codec/array/storage transformers/store workflow. Both were made with https://excalidraw.com. The png files include the source and can be loaded into excalidraw for further editing.
  • Added examples to store terminology section, as requested in
    ZEP0001 - Core v3.0 spec for review #149 (comment)
  • Improved the storage transformer terminology section, as requested in
    ZEP0001 - Core v3.0 spec for review #149 (comment)
  • Reordered the terminology definitions slightly, to fit better to the illustrations
    (moved codec and compressor more to the end, after the second illustration).

Screenshots of the rendered specs with the illustrations

(The changes from the ZEP1 review were applied to the main branch for the screenshots)

Screenshot from 2022-10-04 09-43-20


Screenshot from 2022-10-04 09-43-43

@jstriebel
Copy link
Member Author

This PR replaces the original PR alimanfoo#1, as we discussed to gather all PRs here onto the main branch (see #149 (comment)).

joshmoore
joshmoore previously approved these changes Nov 10, 2022
Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • "Added two illustrations": 👍 and in general 💯 for any visual material since it helps the reader. (Downside will be keeping it in sync long-term)
    • "The png files include the source" Interesting!
  • Added examples to store terminology section 👍
  • Improved the storage transformer terminology section 👍
  • Reordered the terminology definitions slightly 👍

There's still a large re-reading outstanding from my side, but I don't see a reason not to get these in.

rabernat
rabernat previously approved these changes Nov 11, 2022
Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is excellent, particularly the drawings. Thanks so much @jstriebel for these additions, which make the spec much more clear.

I have some minor editorial suggestions.

docs/core/v3.0.rst Outdated Show resolved Hide resolved
docs/core/v3.0.rst Outdated Show resolved Hide resolved
docs/core/v3.0.rst Outdated Show resolved Hide resolved
docs/core/v3.0.rst Outdated Show resolved Hide resolved
MSanKeys963
MSanKeys963 previously approved these changes Nov 11, 2022
@MSanKeys963
Copy link
Member

Thanks for sending this PR, @jstriebel.
The illustrations are excellent, and the text looks clear to me. I have one minor feedback:

If I try to view the illustrations here, I can't view them properly. This is because they don't have a background colour.
Should we add the illustrations with white background?

PS. I don't have a strong opinion on this, as I know these illustrations can be viewed on the web browser just fine. It's just the .md file.

docs/core/v3.0.rst Outdated Show resolved Hide resolved
@jstriebel
Copy link
Member Author

If I try to view the illustrations here, I can't view them properly. This is because they don't have a background colour.
Should we add the illustrations with white background?

@MSanKeys963 Good point, next time I'll add the white background, but I'd leave it as-is for now for simplicity.

@joshmoore
Copy link
Member

next time I'll add the white background, but I'd leave it as-is for now for simplicity.

Unless we eventually enable dark mode for the specs as well ;)

@joshmoore joshmoore merged commit cc7f363 into zarr-developers:main Nov 17, 2022
@jstriebel jstriebel deleted the ZEP0001-apply-storage-transformer-feedback-main branch November 17, 2022 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants