-
Notifications
You must be signed in to change notification settings - Fork 0
docs: 🏗️ add interface for an umbrella extensions class #156
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
docs: 🏗️ add interface for an umbrella extensions class #156
Conversation
There was a problem hiding this 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?
|
@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. |
There was a problem hiding this 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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Signe Kirk Brødbæk <[email protected]>
There was a problem hiding this 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. 👍 👍
…package into docs/expand-on-design-of-extensions-and-config
There was a problem hiding this 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 🎉
Co-authored-by: martonvago <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
# 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`
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