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

Add alias for all CSS specs #564

Closed
wants to merge 9 commits into from
Closed

Add alias for all CSS specs #564

wants to merge 9 commits into from

Conversation

marcoscaceres
Copy link
Collaborator

closes #563

@marcoscaceres
Copy link
Collaborator Author

@tobie, sorry, it's been a while since I worked on SpecRef... I was unsure if I should update "refs/w3c.json" or "biblio.json"? I guess "w3c.json" is right?

Also, I'm unsure what to do about the failing tests. Could you guide me a little bit?

Copy link
Collaborator

@tabatkins tabatkins left a comment

Choose a reason for hiding this comment

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

Everything looks good after the one change to the grid alias.

refs/w3c.json Show resolved Hide resolved
@tobie
Copy link
Owner

tobie commented Oct 30, 2019

This is a great.

The reasons the tests are failing is that some of these aliases have already been added manually, probably both in w3c.json and biblio.json. Removing them manually once should fix it, as long as all of them are aliases (and none actually contain data).

Is there anything preventing us from adding the script to update-all so that it runs with automatic updates?

@marcoscaceres
Copy link
Collaborator Author

Is there anything preventing us from adding the script to update-all so that it runs with automatic updates?

No, that was going to be my next question :)

@tobie
Copy link
Owner

tobie commented Oct 30, 2019

Awesome. Let’s do that. I have a few suggestions on the script itself that I’ll get to right away (for consistency with the rest of this mess🙃) and then we should be good to go.

@tobie
Copy link
Owner

tobie commented Oct 30, 2019

Whereby rest of this mess I mean the whole code base, not your pull request.

@marcoscaceres
Copy link
Collaborator Author

Whereby rest of this mess I mean the whole code base, not your pull request.

It's all beautiful soup :) I also slapped mine together rather quickly, so appreciate the look over.

Copy link
Owner

@tobie tobie left a comment

Choose a reason for hiding this comment

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

I think this makes it more consistent with the rest of the stuff.

node "./scripts/run.js" "./github" "whatwg.json";
node "./scripts/run.js" "./github" "whatwg.json" &&
node "./scripts/scripts/css-alias.js" &&
node "./scripts/list-refs.js"; # regenerate, as CSS aliases may have changed
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you regenerating that list here? This seems like a pretty big smell.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, you really can't do that. You're basically by-passing the whole check to verify that you're not removing entries by running this at this point.

Instead you need to figure out what entries are getting removed and add them automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If, for example:

  • today "css-foo" references "css-foo-1",
  • tomorrow "css-foo" becomes "css-foo-2"

Now all the "css-foo-YYYYMMDD" references need to be updated... this is where is gets complicated. Perhaps "date citing" an an alias shouldn't be allowed. Or, we need to figure out how to retain these old references as more aliases.

Copy link
Owner

Choose a reason for hiding this comment

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

Absolutely. I now remember why trying to fix this index issue in the past had made me sad.

The correct way of solving the problem is stacking everything onto "foo" including version numbers ("foo-1"), rec track status ("foo-1-CR"), and dated releases ("foo-YYYYMMDD") like so:

{
    "css-grid": {
        "versions": {
            "1": {...},
            "2": {...},
            "20110407": {...},
            "20120322": {...},
            "20121106": {...},
            "20130402": {...},
            "20130910": {...},
            "20140123": {...},
            "20140513": {...},
            "20150317": {...},
            "20150806": {...},
            "20150917": {...},
            "20160519": {...},
            "20160929": {...},
            "20170209": {...},
            "20170509": {...},
            "20171214": {...},
            "20180206": {...},
            "20180427": {...},
            "20180628": {...},
            "20180804": {...}
        }
    }
}

Compared to what we have which is more:

{
    "css-grid-1": {
        "versions": {
            "20110407": {...},
            "20120322": {...},
            "20121106": {...},
            "20130402": {...},
            "20130910": {...},
            "20140123": {...},
            "20140513": {...},
            "20150317": {...},
            "20150806": {...},
            "20150917": {...},
            "20160519": {...},
            "20160929": {...},
            "20170209": {...},
            "20170509": {...},
            "20171214": {...}
        }
    },
    "css-grid-2": {
        "versions": {
            "20180206": {...},
            "20180427": {...},
            "20180628": {...},
            "20180804": {...}
        },
    }
}

This is actually hard to do given the existing data set.

Copy link
Owner

Choose a reason for hiding this comment

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

Well, I guess even that has some limitations if there are two concurrent versions releasing new drafts on the same day. :-/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😭... indeed. ok... argh. Ok, it's doable. Need to find all "css-grid-*" can then clone all versions in. Will try tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

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

It might be worth merging this minus the changes to update-all, and really giving the data model some thought before committing to something else, there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, having a dated reference to the unleveled ref doesn't make sense; either it only resolves to a particular version's dates (which then become invalid when a new version comes out and the unleveled ref moves) or it resolves to all of them (and then you can't disambiguate when two versions of a spec come out on the same day). The latter issue is definitely possible, especially now that we have such easier publishing via Echidna; I wouldn't be surprised if it had already happened, actually.

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, having a dated reference to the unleveled ref doesn't make sense;

Agree.

The latter issue is definitely possible, especially now that we have such easier publishing via Echidna; I wouldn't be surprised if it had already happened, actually.

I has... I spotted one already.

Copy link
Owner

Choose a reason for hiding this comment

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

Alright, so we'll have to maintain the existing ones for backwards compat and start anew with the rest. Maybe worth looking into the JSON API while we're at it?

refs/wicg.json Show resolved Hide resolved
scripts/css-alias.js Outdated Show resolved Hide resolved
scripts/css-alias.js Outdated Show resolved Hide resolved
scripts/css-alias.js Outdated Show resolved Hide resolved
scripts/css-alias.js Outdated Show resolved Hide resolved
scripts/css-alias.js Outdated Show resolved Hide resolved
scripts/css-alias.js Outdated Show resolved Hide resolved
scripts/css-alias.js Outdated Show resolved Hide resolved
scripts/css-alias.js Outdated Show resolved Hide resolved
Copy link
Owner

@tobie tobie left a comment

Choose a reason for hiding this comment

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

Really need to fix the aliasing here, so that we don't lose any entries when the script is run.

test/ref-list.json Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Collaborator Author

I'm struggling a bit :( The changes to "test/ref-list.json" would add unwanted refs keyed on version (which we don't want, based on the discussion here).

I'll revert the changes to "test/ref-list.json", but I think we still need to figure out how to change some assumptions we've made with referencing the aliasOf things that are going to break going forward.

I don't know yet how to cope with that... we might be in breaking change situation - or we might need a way of hard coding those legacy aliasOf generated references in "test/ref-list.json".

@marcoscaceres
Copy link
Collaborator Author

Ok... this gets us back into a "good state". However, running "scripts/list-refs.js" again now will generate the references we don't want anymore.

Let's see if we can figure out a good path forward.

@ExE-Boss
Copy link
Collaborator

It should probably now be possible to implement this using the framework introduced in #573.

@marcoscaceres
Copy link
Collaborator Author

@ExE-Boss, excellent! Should this be closed or could you provide me with some guidance for next steps?

@sidvishnoi
Copy link

Any updates on this?
Do we need to manually add all entries to overwrites/w3c.json? Can we safely redirect the output of this script to overwrites/w3c.json to add createAliasentries?

@marcoscaceres
Copy link
Collaborator Author

@tobie, @ExE-Boss, was waiting on next steps on this... @sidvishnoi is willing to take it over, but we need some guidance 🙏

@tobie
Copy link
Owner

tobie commented May 29, 2020

My feeling is that to move forward with something like this will require a lot of refactoring.

The schema we have doesn’t really handle the different ways the different groups think about versioning. I don’t know that there’s a model that would work across the board, but maybe there are a couple?

Once we have a schema in place we‘ll also need to have have a migration story for the existing data and for the generated references.

Do we keep the latter around or do we deprecate them?

We’ll also probably need to rethink how the generated files are kept separated from one another. That has cause more problems than anticipated. Similarly, whether these should contain manual edits or not should be clarified, so should how overwriting works and why, etc.

Maybe there’s a shortcut here, but I’ve attempted it twice and failed both times.

@tobie
Copy link
Owner

tobie commented Jun 1, 2020

In particular, you want to look at this comment and the following thread.

@tobie
Copy link
Owner

tobie commented Aug 4, 2020

Should we close this and reopen something else once we have clarity as to how to move forward?

@sidvishnoi
Copy link

Agree. Lets close this for now.

@tobie tobie closed this Aug 5, 2020
@tobie tobie deleted the css-aliases branch April 26, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarification on CSS Cascade
5 participants