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

Add type and accepted values information #56

Merged
merged 10 commits into from
Dec 4, 2018
Merged

Add type and accepted values information #56

merged 10 commits into from
Dec 4, 2018

Conversation

plehegar
Copy link
Member

Those annotations are intended to be used by validate-repo in the future. It will avoid having to change validate-repo when a new value is added for example for repo-type.

@plehegar plehegar requested a review from tripu October 17, 2018 14:20
@tripu tripu self-assigned this Oct 26, 2018
tripu
tripu previously requested changes Oct 26, 2018
Copy link
Member

@tripu tripu left a comment

Choose a reason for hiding this comment

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

Nice idea! Please see suggestions inline.

Also, I'm wondering if it wouldn't be better to specify the expected types with JSON-LD or JSON Schema syntax, instead of inventing our own.
eg, instead of

number|array<number>

something like this

"anyOf":[{"type":"number"},{"type":"array","items":{"type":"number"}}]

I guess it'll make things easier in validate-repos, if we can use a JSON-LD or JSON Schema library, instead of writing our own parser?

w3c.json.html Outdated Show resolved Hide resolved
w3c.json.html Outdated Show resolved Hide resolved
w3c.json.html Outdated Show resolved Hide resolved
@tripu
Copy link
Member

tripu commented Oct 26, 2018

(@plehegar, can you review #55, please?)

@plehegar
Copy link
Member Author

I like the idea of using a schema instead. But that will mean we'll have to integrate a schema validator. Feel to close this pull request without merging.

@tripu
Copy link
Member

tripu commented Nov 14, 2018

(Looking at this diff now, because the rebase somehow introduced spurious differences in the PR.)

The reusing-a-syntax thing was a suggestion, @plehegar. Better to merge this as it is than abandon it altogether!

Co-Authored-By: plehegar <[email protected]>
@plehegar
Copy link
Member Author

should be ok to merge now I think.

@plehegar plehegar dismissed tripu’s stale review November 28, 2018 21:19

should be a v.next request

Copy link
Member

@tripu tripu left a comment

Choose a reason for hiding this comment

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

See same inline comment.

@tripu tripu merged commit 245eaa7 into master Dec 4, 2018
@tripu tripu deleted the validation branch December 4, 2018 08:08
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.

3 participants