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

Merged
merged 38 commits into from
Jan 27, 2025

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
@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?

@pierrecamilleri
Copy link
Author

npm run generated!
(sorry for the delay)

@johanricher
Copy link
Member

Great work @pierrecamilleri 👏 Looks like it's ready to merge now 🚀

@roll
Copy link
Member

roll commented Jan 27, 2025

Hi @pierrecamilleri,

Sorry for the slow communication due to the holiday season =)

Looks great!

We just need to remove any changes to public/profiles/2.0. from the diff to ensure published standard immutability.

@pierrecamilleri
Copy link
Author

We just need to remove any changes to public/profiles/2.0. from the diff to ensure published standard immutability.

Done! (Sorry I have missed this)

@roll
Copy link
Member

roll commented Jan 27, 2025

Amazing! Thanks!

@roll roll merged commit 3bd8961 into frictionlessdata:next Jan 27, 2025
@roll
Copy link
Member

roll commented Jan 27, 2025

Merged. Here is a live preview of v2.1 - #1051

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
9 participants