Skip to content

Conversation

@lwjohnst86
Copy link
Member

Description

Related to some discussions like in #120 and #122 about how we handle custom checks/extensions to the checks against the Data Package standard.

Closes #152

Needs an in-depth review.

Checklist

  • Formatted Markdown
  • Ran just run-all

@lwjohnst86 lwjohnst86 mentioned this pull request Oct 27, 2025
2 tasks
Copy link
Contributor

@martonvago martonvago left a comment

Choose a reason for hiding this comment

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

The design of the toml file looks good and makes sense to me!

Where I’m struggling a bit is seeing how the Extension class matches / implements this design.

Based on the toml file, I would expect Config to have the fields version, strict, exclusions, and extensions, where an extension is either of type required or custom_check. So one Config can have multiple Extensions, but one Extension can only express one check (whether required or custom).

The design of the Extension class seems to allow multiple checks (whether required or custom) to be expressed in one Extension. Is this correct? Or can you explain the class structure a bit more?

@lwjohnst86
Copy link
Member Author

@martonvago One config can only have one extension class. See the TOML spec for Array of Tables: https://toml.io/en/v1.0.0#array-of-tables.

@lwjohnst86 lwjohnst86 requested a review from martonvago October 27, 2025 10:01
Copy link
Member

@signekb signekb left a comment

Choose a reason for hiding this comment

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

Nice! I've added some suggestions:


``` toml
# The Data Package standard version to check against.
version = "v2"
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to be a string and not a e.g. a float 2.0? At least, we should include the .0 since we know there's a v2.1 on the way.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could provide a set list of potential values in it, like ["v2", "v2.0", "v1", "v2.1"] to allow short hands of v2.

Copy link
Member

Choose a reason for hiding this comment

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

That's true, but I still don't like the ambiguity of "v2" 🤔 Like would that refer to "v2.0" or "v2.1"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it's just that their documentation refers to v2: https://datapackage.org. The only place v2.0 is used is in the Changelog. But everywhere else, it is v2. I'd rather keep aligned with what they use in their language. But I'm also not super strongly opinionated on this.

Copy link
Member

Choose a reason for hiding this comment

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

@lwjohnst86 That's true; they tend to refer to v2 on their website. They do, however, use 2.0 in their profiles, e.g., https://datapackage.org/profiles/2.0/datapackage.json.

@github-project-automation github-project-automation bot moved this from In Review to In Progress in Iteration planning Oct 27, 2025
@lwjohnst86 lwjohnst86 requested a review from signekb October 27, 2025 10:37
@lwjohnst86 lwjohnst86 moved this from In Progress to In Review in Iteration planning Oct 27, 2025
Copy link
Contributor

@martonvago martonvago left a comment

Choose a reason for hiding this comment

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

Ah okay, so [[extensions.required]] creates a structure like "extensions": {"required": [...], "custom_check": [...]} and not an array of extensions (like e.g. [[exclusions]]).

Okay, then I'm happy with this, I'd just prefer the singulars/plurals to be aligned with the structure. 👍 👍

@github-project-automation github-project-automation bot moved this from In Review to In Progress in Iteration planning Oct 27, 2025
@lwjohnst86 lwjohnst86 requested a review from martonvago October 27, 2025 10:59
@lwjohnst86 lwjohnst86 moved this from In Progress to In Review in Iteration planning Oct 27, 2025
Copy link
Contributor

@martonvago martonvago left a comment

Choose a reason for hiding this comment

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

Nice, just one typo 🎉

@github-project-automation github-project-automation bot moved this from In Review to In Progress in Iteration planning Oct 27, 2025
@lwjohnst86 lwjohnst86 requested a review from martonvago October 27, 2025 11:55
@lwjohnst86 lwjohnst86 moved this from In Progress to In Review in Iteration planning Oct 27, 2025
Copy link
Contributor

@martonvago martonvago left a comment

Choose a reason for hiding this comment

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

🎉

@lwjohnst86 lwjohnst86 merged commit c328343 into main Oct 27, 2025
6 checks passed
@lwjohnst86 lwjohnst86 deleted the docs/expand-on-design-of-extensions-and-config branch October 27, 2025 12:36
@github-project-automation github-project-automation bot moved this from In Review to Done in Iteration planning Oct 27, 2025
lwjohnst86 added a commit that referenced this pull request Nov 4, 2025
# Description

As per #156, renaming custom checks to be extensions instead. Will
implement the Extensions class in another PR.

Needs a quick review.

## Checklist

- [x] Ran `just run-all`
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.

Expand on design of extensions in config

4 participants