docs: 🏗️ add interface for an umbrella extensions class#156
docs: 🏗️ add interface for an umbrella extensions class#156lwjohnst86 merged 10 commits intomainfrom
Conversation
martonvago
left a comment
martonvago
left a comment
There was a problem hiding this comment.
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?
|
@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. |
signekb
left a comment
signekb
left a comment
There was a problem hiding this comment.
Nice! I've added some suggestions:
|
|
||
| ``` toml | ||
| # The Data Package standard version to check against. | ||
| version = "v2" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We could provide a set list of potential values in it, like ["v2", "v2.0", "v1", "v2.1"] to allow short hands of v2.
There was a problem hiding this comment.
That's true, but I still don't like the ambiguity of "v2" 🤔 Like would that refer to "v2.0" or "v2.1"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
Bot
moved this from In Review
to In Progress
in Product development
Co-authored-by: Signe Kirk Brødbæk <signebroedbaek@gmail.com>
martonvago
left a comment
martonvago
left a comment
There was a problem hiding this comment.
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
Bot
moved this from In Review
to In Progress
in Product development
…package into docs/expand-on-design-of-extensions-and-config
martonvago
left a comment
martonvago
left a comment
There was a problem hiding this comment.
Nice, just one typo 🎉
github-project-automation
Bot
moved this from In Review
to In Progress
in Product development
Co-authored-by: martonvago <57952344+martonvago@users.noreply.github.com>
# 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`
3 participants
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
just run-all