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

All our JSON files should validate against the same schema #20

Closed
JeremiePat opened this issue Aug 10, 2016 · 7 comments
Closed

All our JSON files should validate against the same schema #20

JeremiePat opened this issue Aug 10, 2016 · 7 comments
Labels
enhancement 🥇 Nice to have features. schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project.

Comments

@JeremiePat
Copy link
Contributor

JeremiePat commented Aug 10, 2016

Hi folks :)

I noticed that our JSON are currently not really coherent, it would be nice if they could validate against the same JSON schema.

After looking at the existing JSON, I'm suggesting the following schema:
https://gist.github.com/JeremiePat/f83f3cb00471c1b90d126fd6989256eb

In short, it defines some meta type and how to aggregate them:

  • Feature set: An aggregation of feature sets or an aggregation of feature with at least one feature with the key basic-support
  • Feature:
    • An optional name (as an l10nString) This is primarily meant to avoid using keys as feature name. IMO object keys should always remain a technical ID and shouldn't be used for display
    • A list of supported platforms. This list is finite (but extensible) and each item is of type browser
    • A mandatory list of meta data that describe the status of the feature
  • Browser: an browser object describe any environment implementing a feature it must contain a support type (either null—for unknown support, boolean—for a basic yes/no support, or string—to provide a version number and can have an optional notes object which is an l10nString)
  • l10nString: This is a generic type to deal with string that can be localized. It can be either a Array of strings (this assume the strings in the area are in English) or an object for which each key is a language code and each value is an array of strings in the corresponding language.

Moving to such schema will obviously require to update our JSON file, I think now is the right time before we get to much data to make such changes.

It also worth noting that such data structure change will require the MDN team to update the Kuma Macros they are using to pull the data.

@JeremiePat JeremiePat changed the title All our JSON file should validate on the same schema All our JSON file should validate against the same schema Aug 11, 2016
@JeremiePat JeremiePat changed the title All our JSON file should validate against the same schema All our JSON files should validate against the same schema Aug 11, 2016
@wbamberg
Copy link
Collaborator

Hi Jeremie

I agree that having a common schema is important, and this is a thing we've been chatting about and slowly working towards. I think your proposal looks great.

Localization

We've talked before about the l10n problem with this JSON. IIRC think we were thinking along similar lines to what you've done here. But I know very little about how to design something that fits the needs of localizers here. This approach implies that all the translations live in the same JSON file, right? Is that desirable, or would it be better to have separate files for different languages? I guess if we have a build step (as per #21) then this would not be a problem: localizers could work in separate files and they could be aggregated.

Granularity/"basic support"

I think I'm a bit confused about granularity here. Coming from WebExtensions, I think of the most atomic things being things like functions, events, types, like tabs.executeScript or webRequest.onAuthRequired. These things have compat tables that include detailed compatibility info in the form of notes. Then they are aggregated into higher level module-type things, like tabs or webRequest, that have similarly-aggregated compat tables, that link to the atomic tables for the full details.

In this application I'd have expected that tabs.executeScript is a feature and tabs is a featureSetLevel1. But then I'd always kind of assumed that "basic support" applies to the atomic things, and on this schema it's applied to feature sets.

For instance, take promise.then(). It has a compat table listing "basic support". Does that mean promise.then() is a feature set? If it is, where are the features?

In general actually, I'm not sure the concept of "basic support" is terribly useful. It seems quite ill-defined - one person's advanced bleeding-edge feature is another person's basic feature, right? Does it have a clear definition and if not, can we remove it, and instead talk about what is actually supported?

@JeremiePat
Copy link
Contributor Author

Hi!
Thanks for the details comment.

Localization

You're right the current approach include the whole localization in the same JSON file as the data. I agree it is not super optimized. At that stage I just want to make sure we are able to localized strings and the current solution is what I end up with to be able to translate the notes that are already there.

I agree that as soon as we end up with #21 it could be a good idea to externalized those strings but I think that at this stage we shouldn't worry to much, as if we already have crude but structured localization mechanism we will be able to make it evolve as we go.

"basic support"

Well, the more I fiddle with the data the more I tend to agree with you. We don't need that odd "basic support" thing as it is either a presentation trick or an aggregated information that can be computed automatically based on the existing data.

Let's keep things simple and let's get rid of that until we have clearer agreement/need about that.

I updated the schema to reflect that change:
https://gist.github.com/JeremiePat/f83f3cb00471c1b90d126fd6989256eb

I also clean what are feature vs featureSet:

  • A feature is the most atomic element we can have to define support (example: Promise.then)
  • A featureSet is an aggregation of feature (example: the whole Promise API)
  • A featureSetLevelN is an aggregation of any feature* objects (example: all the JavaScript features sets)

The reason for N featureSetLevelN is because the JSON schema spec does not provide a built in mechanism to define objects recursively.

@wbamberg
Copy link
Collaborator

Thanks Jeremie, I like this. I had a couple of other comments, one big, one small.

The small one: I'd like to see a definition of the values "status" can take. As I understand it, in this schema a feature can have any combination of status values (for example, it can be both "experimental" and "standardized"). Is this what we want?

The big one: names. You said that "feature" has an optional name, which is an l10nString:

This is primarily meant to avoid using keys as feature name. IMO object keys should always remain a technical ID and shouldn't be used for display

So far I've always considered that feature names are identifiers (e.g. "executeScript"). This makes the schema simpler and easier to read, means I don't have to worry about localization of them, and makes my code simpler too, since I can just do stuff like var featureData = featureSet[featureName]. If the keys aren't to be feature names, I'm not really clear what they should be in my case.

So while I'm open to this change if it has a clear benefit all-round, it would have a negative impact for my own docs, and I'm a bit resistant to adding complexity because we think it might come in handy in the future.

@SamB
Copy link
Contributor

SamB commented Aug 27, 2016

@wbamberg:

This is primarily meant to avoid using keys as feature name. IMO object keys should always remain a technical ID and shouldn't be used for display

So far I've always considered that feature names are identifiers (e.g. "executeScript"). This makes the schema simpler and easier to read, means I don't have to worry about localization of them, and makes my code simpler too, since I can just do stuff like var featureData = featureSet[featureName]. If the keys aren't to be feature names, I'm not really clear what they should be in my case.

I think the point here is to let the keys act as your identifiers, while providing a bit more flexibility for features which are not simply named after the identifiers/keywords used to invoke them, such as "Constructor requires new" in javascript/promise.json. Though I'm really not sure you have many such features yet, and being monolingual in English I really have no idea how useful translations of such things are...

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 3, 2016

Still, this change introduces complexity in the schema, and it's not obvious to me how we can use the same macro code for building compat tables, if some components use the key as an identifier and some use "name". And it seems that having common macro code would be one of the benefits of having a common schema.

So there's definitely a cost to making this change. The question is whether it's worth it at this time.

such as "Constructor requires new" in javascript/promise.json.

I'm not at all familiar with the compat data outside WebExtensions, and it's quite possible that there are lots of good examples of "feature names" which need to be localized. And if that's the case, then that's fine, we'll just have to live with the extra complexity. But "Constructor requires new" doesn't sound like a feature to me, it sounds more like something that would be captured in a compatibility note.

@stephaniehobson
Copy link

The key should always be used as the identifier but name can be in the schema separately.

I think we handled the name vs localized name in the old browser compat project by allowing a string or an object in the name field. So name: 'featurename' was not localized and displayed on all locales and name: { 'en-US': 'English Feature Name', 'fr' : 'French Feature Name' } was localized.

Could some thing similar be done where if name is omitted entirely the key is used and we assume it does not need to be localized?

@Elchi3
Copy link
Member

Elchi3 commented May 4, 2017

I'm closing this as we are about to merge a new schema after a few months of discussions – a schema that aims to be the single validator for all data of this repo. I encourage everyone to open new issues against that new schema. Thanks for your input!

@Elchi3 Elchi3 closed this as completed May 4, 2017
@queengooborg queengooborg added enhancement 🥇 Nice to have features. schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project. labels Aug 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🥇 Nice to have features. schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project.
Projects
None yet
Development

No branches or pull requests

6 participants