-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Adds ability to check for possible updates across multiple default statements #967
Conversation
@ChrisC let me try to summarize succinctly what it is I think we should do here:
|
@foolip Good news! I ran this sample code through the multiple-statement test case and verified that it does work 👍🏽 I think I missed that this loop continues iterating through all the statements until we find a confirmation of support per version, and only returns I'll go ahead and integrate this approach, add the handling for possible booleans/ "preview" values, and remove the support range utility method . |
in favor of simpler support detection across multiple default statements
adds additional boolean & string test cases
for use in the `hasSupportUpdates` checks that run before and after `persist...` operations
@foolip I believe I got in all of the major requested changes. Feel free to take another look at this! |
return false; | ||
} | ||
|
||
// In the case of general boolean statements, only show support if the version from the test result does not show specific support, otherwise we should ignore generic boolean support statements in favor of specific version support info from a test result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree this makes sense. It's a bit unfortunate that hasSupport
is passed in, but it makes sense here. Maybe it can be refactored somehow, but I'm not sure how right now, and this works!
scripts/update-bcd.ts
Outdated
}) => { | ||
if (!statements?.length) { | ||
if (hasSupportUpdates(versionMap, simpleStatement)) { | ||
if (hasSupportUpdates(versionMap, unmodifiedDefaultStatements)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wait, going just by the name, isn't this the original BCD data (without any modifications) and not the updated BCD data that we're about to write back to disk? It needs to be the updated data, but filtered once again to "default statements", meaning excluding things like prefixed statements, flags, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's correct. Seems like I misunderstood the requirement here. We were checking the updated, filtered "default statements" before this change, so I'll walk this back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok addressed here bec9963
I've tested this and there are now very few warnings, only 3:
I took a look at the last one, and found that it was actionable and that fixing it silenced the warning: This is very nice, thanks @ChrisC! |
That's great to hear! |
This continues from the work in #909 and implements a request by @foolip to check for possible updates across multiple default statements in the new preliminary
hasSupportUpdates
check.These new test cases show what happens with multiple default statements with different combinations of test results.
The update script now logs more warnings from cases where a possible update was detected in more complex scenarios, but was not completed by the update script.
cc @foolip