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

tools/internal/parser: enforce suffix ordering in the ICANN section #2295

Merged

Conversation

danderson
Copy link
Contributor

The ordering rule is slightly different from the private section: suffix blocks do not get reordered, only the suffixes within each block. Additionally, the ICANN section contains many load-bearing inline comments. To avoid semantic changes, the sorting does not move suffixes over such comments. Unlike in the private section, such movement failures aren't treated as errors, because the subsection comments in the ICANN block have a reasonably consistent meaning.


This will fail CI until #2287 is merged, along with the additional sorting fixups in https://gist.github.com/danderson/cb0fb30acbec38651cf8c0a1f4275125 for IDNA domains. Depending on whether #2287 is merged with or without the accent fixes, I can add the remaining fixups in this PR, up to you @simon-friedberger @groundcat.

The ordering rule is slightly different from the private section:
suffix blocks do not get reordered, only the suffixes within each block.
Additionally, the ICANN section contains many load-bearing inline comments.
To avoid semantic changes, the sorting does not move suffixes over such
comments. Unlike in the private section, such movement failures aren't
treated as errors, because the subsection comments in the ICANN block
have a reasonably consistent meaning.
@dnsguru
Copy link
Member

dnsguru commented Nov 28, 2024 via email

@simon-friedberger
Copy link
Contributor

Does anybody know how I can rebase this on current main to rerun the checks? It seems Github is checking this PR as if it would be applied to the old main which is just useless.

@wdhdev
Copy link
Contributor

wdhdev commented Nov 29, 2024

Just manually update the branch, if you have the 'Recommend Branch Updates' option enabled in settings it should show up with a prompt to update the branch. Also it won't merge into 'the old main', even with an outdated branch.

@simon-friedberger
Copy link
Contributor

Just manually update the branch, if you have the 'Recommend Branch Updates' option enabled in settings it should show up with a prompt to update the branch. Also it won't merge into 'the old main', even with an outdated branch.

Agreed, but it makes the check a lot less useful if it doesn't actually check the result of merging.

@simon-friedberger simon-friedberger merged commit 70df5fb into publicsuffix:main Nov 29, 2024
1 of 2 checks passed
@danderson
Copy link
Contributor Author

Oops, sorry I didn't see the main branch got renamed. I'll update my clones to match in future.

@danderson danderson deleted the danderson/pr-vpqnvkwmwmss branch November 30, 2024 00:11
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.

4 participants