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.

Sign up for GitHub

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

Jump to bottom

Add spec/schema to implement mod tags #2034

Merged
merged 6 commits into from
Oct 20, 2017
Merged

Add spec/schema to implement mod tags #2034

merged 6 commits into from
Oct 20, 2017

Conversation

smattiso
Copy link
Contributor

@smattiso smattiso commented Apr 20, 2017 •
edited
Loading

This patch is related to issue #1021

@Olympic1 Olympic1 added Discussion needed Enhancement New features or functionality labels Apr 20, 2017
@ayan4m1
Copy link
Contributor

ayan4m1 commented Sep 28, 2017

This looks good to me, it is a simple change which can allow for more complex work to be done. Can the NetKAN guys chime in as to whether or not they like this change? @dbent @Olympic1

@smattiso
Copy link
Contributor Author

smattiso commented Oct 4, 2017

Due to my own concerns that my proposed data structure does not appear similar to other data structures in the CKAN file, I am patching this spec to include these multiple category definitions in an array. Also, as 'type' and 'description' seem like keywords, it's probably best practice to avoid using those as descriptors. I am not sure if it is possible to do it the way I originally had it, but better safe than sorry.

@smattiso
Sort column schema restructure patch
de2776b 
This patch updates the Schema of the sort column to use an object array.
This structure is similar to other structures used in the schema file.
Additionally, the descriptors 'type' and 'description' have been changed
to improve readability, and the spec file has been changed to reflect this.
@dbent
Copy link
Member

dbent commented Oct 5, 2017

My main concern is that there could be a lot of hand-wringing over categories, what they should be, which mods belong in which categories, et cetera. Ontology can be a blood sport to some people. Also, if we have a strict enumeration in the spec that means every new category would have to be accompanied by a revision of the spec. Especially problematic if we want to actually do runtime schema-checking and enforcement.

I'd much prefer quasi-freeform tags (restricted to lowercase alphanumerics plus hyphen, i.e. ^[a-z-]+$) with a list of "standard" tags that people are encouraged (but not restricted) to using.

@ayan4m1
Copy link
Contributor

ayan4m1 commented Oct 5, 2017

Agreed from me - I think the term "tag" rather than "category" is more useful to the end-user, @smattiso are you amenable to making the changes described by @dbent, as I see it mainly consist of changing "category" to "tags"?

@smattiso
Copy link
Contributor Author

Sure thing. I'll have those updates in a jiffy! ...just as soon as I figure out how to spec quasi-freeform tags... 😛

@smattiso
Quasi-freeform tag patch
1d6450f 
This patch alters the structure of the "Sort column" schema to
remove the potential problems with ontology inherent to defining
"Categories", and redefines it into an array of strings called
"Tags" which may be chosen from a predefined list, or custom.

Custom tags are restricted to lowercase alphanumerics and hyphens.
A list of potential predefined tags may be found in prior commits
or in the original issue KSP-CKAN#1021 opened by Andy81le.
@dbent
Copy link
Member

dbent commented Oct 17, 2017

@smattiso You'll want the pattern property (there are already examples of it in CKAN.schema) and use the pattern: ^[a-z0-9-]+$, which is "one or more characters that are a lower case letter, number, or hyphen".

Acceptable:

  • foo
  • foo-bar
  • foo-bar-100

Unacceptable:

  • Foo
  • Foo Bar
  • Foo Bar !!!

@smattiso
Copy link
Contributor Author

0-9! That's what I forgot. 😮

@smattiso
0-9 fix
4abcfa7 
This patch is an addendum to the previous patch, adding the specified
numeric functionality missing from the previous pattern definition.
@dbent
Copy link
Member

dbent commented Oct 17, 2017

0-9! That's what I forgot. 😮

No problem, I think I left it off when I first listed the pattern in the previous comment.

FYI, for anyone reading this in the future, I intentionally left out uppercase letters (A-Z) so we don't have an issue of different casing or whether or not tags should be compared case sensitively or insensitively or any of that other jazz. A tag should have one single, canonical, representation.

@dbent
Copy link
Member

dbent commented Oct 17, 2017

Changes look good to me, but you'll want to update the categories property in the example file in Spec.md.

@smattiso
Copy link
Contributor Author

Haven't had my coffee this morning. *fixed

@smattiso
Example CKAN tags patch
5f179cd 
This patch updates the Example CKAN file section to show example
standard tags and custom alphanumeric tags relevant to the project.
politas
politas reviewed Oct 18, 2017
View reviewed changes
Spec.md Outdated
@@ -372,6 +380,30 @@ This field defaults to `false`, including for `spec_version`s less than
`v1.16`, however CKAN clients prior to `v1.16` would only perform strict
checking.

##### tags

The `tags` field describes keywords that a user or program may use to
Copy link
Member

Choose a reason for hiding this comment

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

Please add a (v1.23) to indicate that this will only be supported by CKAN clients of version v1.23.0 and higher

Copy link
Member

Choose a reason for hiding this comment

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

We always go up by two numbers (14, 16, 18, 20, 22). So shouldn't the next be v1.24?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding 1.24...

@Ayelis
Classification tags in spec as of 1.24
7c63c80 
This patch updates the spec file with classification tags as of
CKAN client versions 1.24.0 or higher.
@smattiso
Copy link
Contributor Author

🙄 That's what I get for logging in with my laptop...
That last commit was mine, y'all, FWIW.

@politas politas changed the title Add spec/schema to implement category sort column Add spec/schema to implement mod tags Oct 20, 2017
@politas politas merged commit 7c63c80 into KSP-CKAN:master Oct 20, 2017
politas added a commit that referenced this pull request Oct 20, 2017
@politas
Merge #2034 Add spec/schema to implement mod tags
bd1c5da
@politas politas mentioned this pull request Oct 20, 2017
Closed
5 tasks
@smattiso smattiso mentioned this pull request Oct 20, 2017
Closed
@Olympic1 Olympic1 added this to the 1.24.0 milestone Nov 14, 2017
@HebaruSan HebaruSan mentioned this pull request Oct 9, 2018
Merged
@HebaruSan HebaruSan mentioned this pull request Dec 9, 2019
Merged
@DasSkelett DasSkelett mentioned this pull request Feb 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Olympic1 Olympic1 Olympic1 left review comments

@politas politas politas left review comments

Assignees

@ayan4m1 ayan4m1

@politas politas

Labels
Discussion needed Enhancement New features or functionality
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.
7 participants
@smattiso @ayan4m1 @dbent @politas @Olympic1 @pjf @Ayelis