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

Update tags section of spec #2991

Merged
merged 1 commit into from
Feb 7, 2020
Merged

Conversation

HebaruSan
Copy link
Member

The spec has a tags section, but it's mostly inaccurate and misleading. Now it's brought up to date with how the tags actually work.

@HebaruSan HebaruSan added Pull request Spec Issues affecting the spec labels Feb 7, 2020
@HebaruSan HebaruSan requested a review from DasSkelett February 7, 2020 17:40
@DasSkelett
Copy link
Member

I was wondering about the spec version for the tags.
They are part of the schema since a long time (#2034, included in CKAN v1.24),
but on the other hand the client uses them only since v1.26.8, but didn't throw any errors or was missing anything vital when encountering them before that version.
Netkan has no special handling for the tags apart from sorting them at a specific location.

The spec isn't really clear in this case:

A vx.x string (eg: "v1.2"), being the minimum version of the reference CKAN client that will read this file.

Would it make sense to set the spec version for tags back to v1.24 as it was originally in the #2034, when they where officially introduced in the schema?
Otherwise shouldn't it be v1.27, because the client from v1.26.0 - v1.26.6 didn't do anything with them yet?

@HebaruSan
Copy link
Member Author

Yes, I see no perfect answer here.

  • A module containing tags can be loaded on any client and will simply be ignored on the older ones, so the spec could just omit a spec version indicator
  • It was indeed added to the spec in 1.24, but the spec could have said v1.24 back then if that was the right answer, instead v<TBA> was chosen
  • If we can't replace <TBA> with a real version now, then we'll never do it
  • If the reader is looking at the spec to understand when the functionality becomes useful, then v1.26 is the closest right answer. Yes, some 1.26 versions ignore it as 1.24 and 1.25 did, but the functionality finally landed on this version series.

@DasSkelett
Copy link
Member

Okay. I'd still lean more towards v1.27, because a reader can be sure that they are recognized from this version up, and the support in v1.26.8+ is basically a bonus. Otherwise we make v1.26.0 - v1.26.6 retroactively not conformant to the spec.
But I'm okay to go with either solution.

@politas
Copy link
Member

politas commented Feb 7, 2020

I think the idea is that the spec version is the minimum level of client that won't mess things up for a given element. Ignoring tags is perfectly correct behaviour.

@HebaruSan
Copy link
Member Author

Otherwise we make v1.26.0 - v1.26.6 retroactively not conformant to the spec.

OK, I'm convinced. Changed to 1.27.

@HebaruSan
Copy link
Member Author

I think the idea is that the spec version is the minimum level of client that won't mess things up for a given element. Ignoring tags is perfectly correct behaviour.

@politas, which value would you recommend we use based on that?

@politas
Copy link
Member

politas commented Feb 7, 2020

So we're making Bussard v1.27.0, then?

@HebaruSan
Copy link
Member Author

That's also open to discussion. I was going to use 1.26.12, but it will be 1.27.0 if you say so.

@politas
Copy link
Member

politas commented Feb 7, 2020

If we're bumping the spec, we should be bumping the client version to match, otherwise the client is not claiming to be compliant with the spec.

@politas
Copy link
Member

politas commented Feb 7, 2020

That is assuming that we have tag support working in the client, now. We do, don't we? I haven't actually looked at that bit, myself.

@HebaruSan
Copy link
Member Author

It was added in #2936, released in v1.26.8.

@DasSkelett
Copy link
Member

DasSkelett commented Feb 7, 2020

We're not really bumping the spec version itself, just the version of one property is "in the future".
I'd argue the client comforms to the spec if it has additional features, but not if it is missing features. So if it supports the tags, while they are not in the spec yet (or not in the current spec version), it's okay, but if the spec says it supports tags, but if the client doesn't (or ignores them), it's not conforming to the spec.

But I guess making the next release v1.27 would solve most of that.

@politas
Copy link
Member

politas commented Feb 7, 2020

Makes sense to me to make tags v1.27 in the spec and make Bussard v1.27.0. Let's go with that.

@HebaruSan
Copy link
Member Author

Sounds good. This PR updates the spec to say tags are in 1.27, and Bussard will be 1.27.0.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Right, almost forgot to approve this PR then :D

HebaruSan added a commit that referenced this pull request Feb 7, 2020
@HebaruSan HebaruSan merged commit efcba4c into KSP-CKAN:master Feb 7, 2020
@HebaruSan HebaruSan deleted the fix/tags-spec branch February 7, 2020 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Spec Issues affecting the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants