-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
There was a problem hiding this 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 👍
docs/data-guidelines.md
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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):
- We have data for major releases and we choose not to include data for prereleases (such as Safari TPs, Microsoft Edge preview builds, etc.).
- 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).
- 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.
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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 💯
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.