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

Promote "Metadata in Table Schema" recipe to the specs. #961

Open
wants to merge 36 commits into
base: next
Choose a base branch
from

Conversation

pierrecamilleri
Copy link

@pierrecamilleri pierrecamilleri commented Jul 5, 2024

Context and related issue :
fix #899

Migrated pull request from datapackage-v2-draft repo

Still to do on this PR:

  • Move table schema to /standards/table-schema.md (cf. main)
  • Move changelog to /overview/changelog.md (cf. main)
  • Update wording so it aligns with Data Package (my pending PR)
  • Update some links
  • Fix invalid json example (it was an unintended change)
  • Clarify if examples are borrowing title and path from Data Resource (I think they are, included in my PR) or if this is something completely separate.

@roll
Copy link
Member

roll commented Jul 8, 2024

Thanks a lot for rebasing the PR @pierrecamilleri !

@pierrecamilleri
Copy link
Author

@peterdesmet @roll

Almost there.

I added "partial" to qualify "Data Resource" in the suggestion of Peter concerning the documentation of top-level "examples" ; because an object with title and path is not a valid Data Resource (name is missing). Or do we want to impose a name property ? What do you think of this wording ?

Copy link
Member

@peterdesmet peterdesmet left a comment

Choose a reason for hiding this comment

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

@pierrecamilleri thanks for the updates! Regarding:

Or do we want to impose a name property ? What do you think of this wording ?

I think we should simply stick to the Data Resource spec and require a name (over a title). I have made suggestions to the files to reflect this change.

profiles/source/dictionary/common.yaml Outdated Show resolved Hide resolved
profiles/source/dictionary/schema.yaml Outdated Show resolved Hide resolved
profiles/source/dictionary/schema.yaml Outdated Show resolved Hide resolved
profiles/source/dictionary/schema.yaml Outdated Show resolved Hide resolved
profiles/source/dictionary/schema.yaml Outdated Show resolved Hide resolved
profiles/source/dictionary/schema.yaml Outdated Show resolved Hide resolved
profiles/target/2.0/datapackage.json Outdated Show resolved Hide resolved
Copy link
Member

@peterdesmet peterdesmet left a comment

Choose a reason for hiding this comment

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

Nice work @pierrecamilleri I did the following:

  • List the properties in the same order as on Data Package: name, title, description, homepage, version, created, keywords, contributors, examples
  • Made some review comments regarding phrasing. I think we can remove the cf. Data Package everywhere, but indicate where it inherits from Data Package where applicable.
  • Minor update to simplify the CHANGELOG

I've started to use these metadata properties, so I'm looking forward to see this adopted!

content/docs/standard/table-schema.md Outdated Show resolved Hide resolved
content/docs/standard/table-schema.md Outdated Show resolved Hide resolved
content/docs/standard/table-schema.md Outdated Show resolved Hide resolved
content/docs/standard/table-schema.md Outdated Show resolved Hide resolved
content/docs/standard/table-schema.md Outdated Show resolved Hide resolved
content/docs/standard/table-schema.md Outdated Show resolved Hide resolved
profiles/target/2.0/datapackage.json Outdated Show resolved Hide resolved
content/docs/standard/table-schema.md Outdated Show resolved Hide resolved
content/docs/standard/table-schema.md Outdated Show resolved Hide resolved
profiles/source/dictionary/schema.yaml Outdated Show resolved Hide resolved
@pierrecamilleri
Copy link
Author

pierrecamilleri commented Sep 25, 2024

Thanks for the quality improvements !

I think we can remove the cf. Data Package everywhere,

Cf. Data Package was more to avoid repeating stuff in the documentation, for instance the datapackage.version gives precision about "It SHOULD conform to the Semantic Versioning requirements and SHOULD follow the Data Package Version recipe." that we probably don't want to repeat. So I think these links add some clarity.

WDYT ?

but indicate where it inherits from Data Package where applicable.

Concerning the inheritance, I wonder if we could not rather drop this mention of inheritance, even for version and contributors. As a user of e.g. frictionless-r or frictionless-py, this would create the expectation that if I access the version or contributors property of a schema, it automatically fills a missing field if the schema is part of a package.

I am not quite sure there is a need for this. The metadata is optional and every creator of datapackage can gauge whether it makes sense to repeat this information, and tooling can choose to look at datapackages properties when relevant.

If you think that dropping it would create confusion, OK for adding it to keywords as well.

@roll roll changed the base branch from main to next October 28, 2024 10:15
Copy link
Member

@roll roll left a comment

Choose a reason for hiding this comment

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

Great work @pierrecamilleri !

@pierrecamilleri
Copy link
Author

I merged the next branch to resolve merge conflicts. In the changelog, I included the changes under the v2.1 header (which release PR is still in Draft status), I hope it is planned to release them under this version number. I corrected the linting errors as well.

@pierrecamilleri
Copy link
Author

Hi all, we added you as a reviewer as you are a voting member and this PR is tagged "Requires 6 voting approvals" ! We would like this PR to land soon, if you can find the time to make a review it would be great !

Copy link
Member

@PietrH PietrH left a comment

Choose a reason for hiding this comment

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

Looks good, I made some small textual suggestions.

content/docs/standard/table-schema.mdx Outdated Show resolved Hide resolved
content/docs/standard/table-schema.mdx Outdated Show resolved Hide resolved
Copy link

@khughitt khughitt left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

Copy link
Contributor

@ezwelty ezwelty left a comment

Choose a reason for hiding this comment

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

End-of-year crunch so admittedly only a cursory review, but looks good to me.

Copy link
Contributor

@pschumm pschumm left a comment

Choose a reason for hiding this comment

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

This looks ok to me.

pierrecamilleri and others added 2 commits December 20, 2024 15:29
Co-authored-by: Pieter Huybrechts <[email protected]>
Apply review suggestions
@pierrecamilleri
Copy link
Author

Thank you very much all for your quick review ! 🙏

@roll except for this last small wording decision, everything looks ready to me!

@roll
Copy link
Member

roll commented Dec 23, 2024

Thanks a lot @pierrecamilleri!

I already voted yes. Can you please run npm run generate having the next branch merged and up-to-date so it will applies changes to v2.1 of the profiles instead of v2.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promote "Metadata in Table Schema" recipe to the specs
8 participants