Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the previous sorting logic for affected ranges and preserves the order they appear in the version constraint, and updates multiple OSV advisory JSON files to match the new ordering.
- build_affected_ranges now directly returns ranges in original order, dropping the semver-based sort helpers
- Unused sorting helper functions (
event_to_semver_for_sorting,semver_for_sorting) are removed - Advisory JSON files are re-ordered to reflect constraint order and one timestamp is updated
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/generate_osv_advisories.py | Simplify build_affected_ranges, remove sorting helpers |
| advisories/persistent_login/osv-sa-contrib-2024-044.json | Re-order ranges to match constraint order |
| advisories/panels/osv-sa-contrib-2025-033.json | Update modified timestamp |
| advisories/eca/osv-sa-contrib-2025-031.json | Re-order ranges to match constraint order |
| advisories/core/osv-sa-core-2023-005.json | Move three version ranges earlier (dedupe bottom section) |
Comments suppressed due to low confidence (3)
scripts/generate_osv_advisories.py:291
- [nitpick] Add a docstring to
build_affected_rangesclarifying that it preserves the order of ranges as they appear in the original version constraint.
def build_affected_ranges(sa_advisory: drupal.Advisory) -> list[osv.Range]:
scripts/generate_osv_advisories.py:294
- [nitpick] Add a unit test to verify that
build_affected_rangesreturns affected ranges in the exact order they appear in thefield_affected_versionsstring.
return [
scripts/generate_osv_advisories.py:298
- Remove any remaining imports related to
semverandtyping.Anyif they are no longer used after deleting the sorting helper functions.
def semver_for_sorting(semver: typing.Any) -> str:
jonholdsworth
approved these changes
May 22, 2025
jonholdsworth
left a comment
jonholdsworth
left a comment
There was a problem hiding this comment.
If that sorting order is indeed guaranteed, this is nice, getting rid of unnecessary code is good.
nzlaura
approved these changes
May 22, 2025
Unifex
approved these changes
May 25, 2025
Unifex
left a comment
Unifex
left a comment
Collaborator
There was a problem hiding this comment.
Less code is good. The reason makes sense for me too.
LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I think this is slightly better since if you're comparing against the original version constraint you in theory don't need to search the whole constraint for the particular range, but really the motivation here is that the current sorting function is a bit choppy from a typing perspective - I was initially going to refactor it, but then realized there's not much difference so here we are.
The order of events in each range should not change nor need explicit sorting as I'm 98% sure it's impossible to express a different order unless we have a bug which is its own issue anyway
I'm happy to discuss different sort orders if people have opinions, but also we can revisit this later if needed too 🤷