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

dandi-schema readme edit #152

Merged
merged 5 commits into from
Dec 9, 2022
Merged

dandi-schema readme edit #152

merged 5 commits into from
Dec 9, 2022

Conversation

melster1010
Copy link
Contributor

Edit readme for stylesheet compliance/grammar/clarity/consistency.

@melster1010 melster1010 added the documentation Changes only affect the documentation label Dec 5, 2022
@melster1010 melster1010 requested review from satra and waxlamp December 5, 2022 14:56
@melster1010 melster1010 self-assigned this Dec 5, 2022
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Base: 96.61% // Head: 96.61% // No change to project coverage 👍

Coverage data is based on head (caa4ef2) compared to base (6e44105).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #152   +/-   ##
=======================================
  Coverage   96.61%   96.61%           
=======================================
  Files          16       16           
  Lines        1623     1623           
=======================================
  Hits         1568     1568           
  Misses         55       55           
Flag Coverage Δ
unittests 96.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@waxlamp waxlamp left a comment

Choose a reason for hiding this comment

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

Text looks good. My suggestions are mostly to do with formatting/style.

README.md Outdated Show resolved Hide resolved
## Description

Every `Dandiset` and associated asset has a metadata object that can be retrieved using
the DANDI API. This library helps create and validate DANDI schema-compliant metadata for `Dandisets`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
the DANDI API. This library helps create and validate DANDI schema-compliant metadata for `Dandisets`
the DANDI API. This library helps create and validate DANDI schema-compliant metadata for Dandisets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we formatted "Dandisets" consistently that way throughout the handbook, using the code element, so I was following suit

Copy link
Member

Choose a reason for hiding this comment

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

Then I agree that, for consistency, it should be formatted as a code element.

But on a broader level, I don't think that is the right choice. Can you point me to the style policy/discussion where this decision was made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was never any discussion or decision made, it had been consistently styled that way before I came on board so I followed that set standard; in hindsight I agree it's not a great idea to use the code element as way to emphasize a term that isn't code! we have an entry in the stylesheet for spelling the term with an initial cap, we can revisit and decide whether to also universally remove the code formatting

Copy link
Member

Choose a reason for hiding this comment

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

Let's make the decision now to remove the code element from the term "Dandiset". Can you update the stylesheet, @melster1010?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@waxlamp sure - I'll have to update the stylesheet, remove the special formatting from "Dandiset" in the Handbook files, as well as this readme (as separate PR)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
melster1010 and others added 4 commits December 7, 2022 15:38
Co-authored-by: Roni Choudhury <2903332+waxlamp@users.noreply.github.com>
Co-authored-by: Roni Choudhury <2903332+waxlamp@users.noreply.github.com>
Co-authored-by: Roni Choudhury <2903332+waxlamp@users.noreply.github.com>
@melster1010 melster1010 requested a review from waxlamp December 7, 2022 21:05
@melster1010 melster1010 merged commit d34658c into master Dec 9, 2022
@melster1010 melster1010 deleted the readme-schema branch December 9, 2022 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Changes only affect the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants