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

Bikeshed doesn't catch references to local definitions that are being removed #2231

Open
annevk opened this issue Feb 21, 2022 · 5 comments
Open

Comments

@annevk
Copy link
Collaborator

annevk commented Feb 21, 2022

We've had a couple of instances in the WHATWG where we remove a definition and it passes CI. But then later once the draft is indexed again it no longer passes CI as there are lingering local references to the removed definition. Ideally Bikeshed would be able to exclude the standard it is currently processing when it falls back to do a global lookup for a definition.

(whatwg/quirks#65 was the latest such instance.)

cc @zcorpan @domenic

@tabatkins
Copy link
Collaborator

It's absolutely supposed to do that already; that's been built in since the very beginning.

If you can provide some specific examples I'll figure out what's up.

@annevk
Copy link
Collaborator Author

annevk commented Feb 21, 2022

whatwg/quirks#58 removed a definition and CI didn't trip (despite there being a reference left to "quirky colors"). Later CI did trip and it turned out whatwg/quirks#66 was needed.

I have seen this happen before, but unfortunately didn't take note of it. If I see it again I'll update this thread. Perhaps Simon and Domenic can recall other examples.

@jyasskin jyasskin added the bug label Dec 14, 2022
@jyasskin jyasskin added this to the Improve reference handling milestone Dec 14, 2022
@jyasskin jyasskin removed this from the Improve reference handling milestone Jan 31, 2023
@jyasskin
Copy link
Collaborator

I believe this happened in WICG/turtledove@bea14ab#diff-6f5a1d8263b0b0c42e2716ba5750e3652e359532647ac934c1c70086ae3ceddaR119. That commit moves some fields from AuctionAdInterestGroup to a new base dictionary GenerateBidInterestGroup and fails to update references to them, but the build kept passing until the next commit at https://github.com/WICG/turtledove/actions/runs/5316854654/jobs/9626781695, which didn't touch those fields.

qingxinwu pushed a commit to qingxinwu/turtledove that referenced this issue Jun 20, 2023
AuctionAdInterestGroup inherits GenerateBidInterestGroup, need to use
{{GenerateBidInterestGroup/xxx}} to reference inherited fields.

speced/bikeshed#2231 covers the bug why the
original CL that made the change didn't catch this error.
@tabatkins
Copy link
Collaborator

Ah yes, I figured out what the problem for this was - I actually broke this six years ago, when I moved to lazy-loading the data files. I no longer correctly remove the "this spec, but from the datafiles" anchors so Bikeshed'll happily link to terms defined by the current public version (until you publish the new version, and it gets respidered...)

@tabatkins
Copy link
Collaborator

(It's on the shortlist of things to fix in Q3.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants