Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

Avoid adding ranges in update-bcd where current data already satisfies them #1068

Closed
gsnedders opened this issue Mar 19, 2021 · 3 comments · Fixed by #1088
Closed

Avoid adding ranges in update-bcd where current data already satisfies them #1068

gsnedders opened this issue Mar 19, 2021 · 3 comments · Fixed by #1088
Labels
Component: update-bcd BCD updater script Priority 2 (High) High priority, should be tackled soon

Comments

@gsnedders
Copy link
Contributor

e.g. don't do this:

-            "safari": {
-              "version_added": "1",
-              "version_removed": "10"
-            },
+            "safari": [
+              {
+                "version_added": "≤6",
+                "version_removed": "10"
+              },
+              {
+                "version_added": "1",
+                "version_removed": "10"
+              }
+            ],
@foolip
Copy link
Owner

foolip commented Mar 19, 2021

Absolutely! Combining two arrays of support statements is surprisingly hard and there are lots of cases not handled, and some are handled poorly. I've documented some of what's wrong in #571.

But this is a case I think must be possible to fix without changing the overall approach.

Note that the reason a lot of weird changes like this are left is because we've mostly been picking the good stuff from the changes produced, and not gotten anywhere near burning down all proposed changes to find all issues like this.

@foolip foolip added Component: update-bcd BCD updater script Priority 2 (High) High priority, should be tackled soon labels Mar 19, 2021
@foolip
Copy link
Owner

foolip commented Mar 19, 2021

Note to self: this is happening for api.Document.createEntityReference.

foolip added a commit that referenced this issue Mar 29, 2021
The previous logic for finding a `simpleStatement` considered anything
with a `version_removed` to be non-simple, going down the code path of
adding entries unless an identical one already existed.

Instead, limit this case to when there were no existing statements, in
which case there's no merging of entries to worry about.

Defer dealing with multiple existing statements or multiple inferred
statements beyond the simple case of no existing statements.

The resulting edits suggest differ by quite a lot from the previous
state, overall updating fewer files.

Fixes #1068.

#875 is the plan for
catching differences in the data which cannot yet be fixed
automatically.
foolip added a commit that referenced this issue Mar 29, 2021
The previous logic for finding a `simpleStatement` considered anything
with a `version_removed` to be non-simple, going down the code path of
adding entries unless an identical one already existed.

Instead, limit this case to when there were no existing statements, in
which case there's no merging of entries to worry about.

Defer dealing with multiple existing statements or multiple inferred
statements beyond the simple case of no existing statements.

The resulting edits suggest differ by quite a lot from the previous
state, overall updating fewer files.

Fixes #1068.

#875 is the plan for
catching differences in the data which cannot yet be fixed
automatically.
foolip added a commit that referenced this issue Mar 29, 2021
The previous logic for finding a `simpleStatement` considered anything
with a `version_removed` to be non-simple, going down the code path of
adding entries unless an identical one already existed.

Instead, limit this case to when there were no existing statements, in
which case there's no merging of entries to worry about.

Defer dealing with multiple existing statements or multiple inferred
statements beyond the simple case of no existing statements.

The resulting edits suggest differ by quite a lot from the previous
state, overall updating fewer files.

Fixes #1068.

#875 is the plan for
catching differences in the data which cannot yet be fixed
automatically.
@foolip
Copy link
Owner

foolip commented Mar 29, 2021

This should no longer be happening after #1088. The script is now a bit more conservative, so there may also be correct edits no longer being made. The total amount of changes makes this hard to vet, but I didn't find anything concerning when sampling the diff before and after the change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Component: update-bcd BCD updater script Priority 2 (High) High priority, should be tackled soon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants