Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Merge specification status, names and urls into a single source #557

Merged
merged 4 commits into from
Jan 26, 2018

Conversation

dontcallmedom
Copy link
Contributor

The purpose of extracting it is to make it easier to validate and update it - I have a number of ideas and tools that I can apply to this, but it's much easier to apply them if the data is cleanly separated.

See also mdn/data#157

@teoli2003
Copy link
Member

We have discussed this in December in Austin and the outcome was that we don't want to make a change to the kumascript macro using this data without writing tests for them before hands. Such tests are a prerequisite before going forward here.

@dontcallmedom
Copy link
Contributor Author

I see there are already some basic tests for spec2.ejs and SpecName.ejs.

Is there a description of what additional tests would be needed to enable such a change?

@jwhitlock
Copy link
Contributor

I agree with the goal, but I think that

  1. We should take multiple steps, by first extracting the data from the spec2 and SpecName templates on the KumaScript side, then sourcing that data from the data package
  2. I think the specification data should be combined in a single source, rather than split up based on what goes in what column in the current table implementation.

I've gone into more detail at mdn/data#158 (comment).

@dontcallmedom
Copy link
Contributor Author

I have updated the pull request to keep the data local for now

@jwhitlock
Copy link
Contributor

The approach looks good to me. I'd appreciate a 👍 from other MDN staffers to the idea. I haven't reviewed the code yet, but I'm willing if we want to go this way.

@dontcallmedom - are you comfortable rebasing to resolve conflicts?

@jpmedley
Copy link
Contributor

I also have some ideas about how this data could be used if it were extracted. I want to repeat the request for what additional tests we would need.

See https://github.com/mdn/data commit f887785798be1c896148f8edc1c415aceafa3ae3
See https://github.com/mdn/data commit c7dea7da24f7417420204ce19c2c48c7feb1fc16
Also merge status, names and urls in a single source
based on feedback at mdn/data#158 (comment)
@dontcallmedom
Copy link
Contributor Author

rebased

@Elchi3 Elchi3 changed the title Extract data on specs and move it to data repository Merge specification status, names and urls into a single source Jan 24, 2018
@jwhitlock
Copy link
Contributor

I see @Elchi3 gave a 👍 to my comment mdn/data#158 (comment) and touched the bug, let's proceed. @teoli2003, please add any details you can remember about the additional requirements.

We have passing test suites for the spec2 and specname macros, so I have more confidence this worked. We should also get an error if the JSON in SpecData.json is ill-formed, so that should help.

I'd like:

  • A test for SpecData that verifies that each element has name, url, and status. That might be better formalized as a JSON schema and validation, once it moves to the data repo.
  • Validate that status is one of the accepted values. Again, this may be better as a schema test.
  • Test that the url is reachable

I'm considering if the alternate keys ("CSS3 2D Transforms" is an older name for "CSS3 Transforms") should be in the JSON, like:

{
    "CSS3 Transforms": {
        "old_keys": ["CSS3 2D Transforms"],
        "name": "CSS Transforms Level 1",
        "url": "https://drafts.csswg.org/css-transforms/",
        "status": "WD"
    },
}

It depends if we see these alternate keys as an accident of MDN history or a useful long-term feature.

I think all those could be done as follow-on PRs, so I'm accepting the PR, and will wait for @Elchi3 or @teoli2003 to merge.

@jwhitlock
Copy link
Contributor

jwhitlock commented Jan 26, 2018

After thinking about it, the "extra tests" are probably those needed to switch to using the mdn-data NPM package (PR #343), rather than direct calls to GitHub-hosted data. This PR is a good step in that direction, and doesn't need to wait for the switch to the NPM package.

@jwhitlock jwhitlock merged commit 0279c28 into mdn:master Jan 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants