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

Ensure spec PR's diffs are generated correctly #455

Open
mbrodesser-Igalia opened this issue Feb 26, 2024 · 5 comments
Open

Ensure spec PR's diffs are generated correctly #455

mbrodesser-Igalia opened this issue Feb 26, 2024 · 5 comments
Milestone

Comments

@mbrodesser-Igalia
Copy link
Collaborator

See #450 (comment).

@koto
Copy link
Member

koto commented Feb 26, 2024

I traced where the spurious diff changes are coming from. Bikeshed generates IDs for term references for the definition panels using "circled digits" (id="ref-for-trusted-types-sink-group②). The circled digits are just sequential numbers, so if a PR refers to a term, or stops to referring to it, all subsequent links of that term will have different IDs, and will surface in a diff.

Unfortunately, I don't think this can be changed. Setting Use Dfn Panels to false still makes Bikeshed generate the IDs, and the actual diff engine doesn't have any relevant configuration. I'm not sure anything can be done about it, short of writing a new postprocessor and registering it with pr-preview.

@koto koto closed this as completed Feb 26, 2024
@mbrodesser-Igalia
Copy link
Collaborator Author

I traced where the spurious diff changes are coming from. Bikeshed generates IDs for term references for the definition panels using "circled digits" (id="ref-for-trusted-types-sink-group②). The circled digits are just sequential numbers, so if a PR refers to a term, or stops to referring to it, all subsequent links of that term will have different IDs, and will surface in a diff.

Unfortunately, I don't think this can be changed. Setting Use Dfn Panels to false still makes Bikeshed generate the IDs, and the actual diff engine doesn't have any relevant configuration. I'm not sure anything can be done about it, short of writing a new postprocessor and registering it with pr-preview.

CC @domenic given your experience with writing specs, did you stumble over that and can recommend a fix?

@domenic
Copy link

domenic commented Feb 26, 2024

I don't really use the diffs, sorry.

@mbrodesser-Igalia
Copy link
Collaborator Author

Unfortunately the diffs still aren't correct. See e.g. #477.

@annevk: are you familiar with this issue and can recommend a fix, or someone who can?

@annevk
Copy link
Member

annevk commented Mar 13, 2024

You could try patching https://github.com/w3c/htmldiff-ui but I'd recommend just skipping past the noise.

(In particular, to do a thorough review you need to look at the source changes so you can use those as a guide as to what constitutes noise.)

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

No branches or pull requests

4 participants