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

Document data conventions #4467

Merged
merged 9 commits into from
Jul 15, 2019
Merged

Document data conventions #4467

merged 9 commits into from
Jul 15, 2019

Conversation

ddbeck
Copy link
Collaborator

@ddbeck ddbeck commented Jul 8, 2019

This is for #4177; see that issue for details. The summary is that there are a number of conventions we use in recording data that cannot be reliably linted. This new documentation provides a single place for contributors to look for such conventions, a place for reviewers to refer to during reviews, and a process for discussing and accepting a new convention: making a pull request adding to the file.

@ddbeck ddbeck added the docs Issues or pull requests regarding the documentation of this project. label Jul 8, 2019
@ddbeck ddbeck requested a review from Elchi3 July 8, 2019 14:16
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thanks for this! I love to have a place for this stuff. Also great to see links to how we came to certain decisions. Well done!
I have one minor nit and one comment on the nodejs section. Otherwise this is a well written foundation and we can add to it nicely in the future 👍

This decision was made in [#3953, under the expectation that most users are likely to run the latest minor version of their browser](https://github.com/mdn/browser-compat-data/pull/3953#issuecomment-485847399), but not necessarily the latest version overall.


## Node.js releases before 1.0
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this section is needed. I had to read it twice to understand what you are referring to. Maybe it is because nodejs before 1.0 is quite rare and I wasn't dealing with this much in PRs.

I think the core nodejs versions confusion in #3160 is that we have a "guideline" for how to do versioning with ECMAScript features (allowing only going after v8 versions) and that other nodejs versions can only be used for non-ECMAScript features (not even sure it is a guideline as we will validate this in linting in #4264). However, it is a somewhat complex situation that should probably be explained somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this conflates a few things, but we don't really spell any of it out anywhere. There's a few facets to this, but I think the section, as written, only addresses the third (and badly):

  1. We have data for major releases and we choose not to include data for prereleases (such as Safari TPs, Microsoft Edge preview builds, etc.).
  2. We make an exception for browsers' prereleases where there's a well-established release process such that the unstable versions are expected to eventually become stable (i.e., Firefox and Chrome's betas and nightlies).
  3. Some Node.js releases look like prereleases (i.e., 0.10 and 0.12) but aren't really because of Node.js-specific history and that's why we allow them.
  4. Some non-major version number releases of Node.js include a new version of V8 that includes an ECMAScript change and we allow those versions too.

All of these things have to do with qualitative distinctions about the data we introduce into browsers/*.json and I don't think the linter can help us with any of these.

I could replace this section with an overview of browser versioning in general, break these down into different sections, or not bother with any of it and chalk this up to us making judgement calls for each of the releases, as they're added.

Copy link
Member

Choose a reason for hiding this comment

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

I think all 4 points need some more thoughts. I would vote to remove the section for now and file this discussion as a follow-up. It's worth digging into some of these things on their own in more details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, thanks. I've dropped the section from this PR. And opened #4492.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thanks Daniel! Happy to see this written up now 💯

@Elchi3 Elchi3 merged commit 5349f9e into mdn:master Jul 15, 2019
@ddbeck ddbeck mentioned this pull request Jul 15, 2019
4 tasks
@ddbeck ddbeck deleted the data-policy branch July 15, 2019 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues or pull requests regarding the documentation of this project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants