Lint: add version consistency check #3368
Conversation
|
Hey, thanks for coming back to this! This is quite big and will need some serious reviewing. I will see how we can get to this. Thank you for some patience in advance :) |
Elchi3
added
the
linter
There was a problem hiding this comment.
Thanks for your effort on this, @vinyldarkscratch! This is a major undertaking and I am impressed that you've made such substantial progress on it.
I've taken a look at the first dozen files in the diff to get a sense for the size and type of changes here. Which is to say, I haven't looked at anywhere near everything yet. My biggest concern so far is that there appears to be a lot of top-level changes which don't have obvious antecedents to be inconsistent with. So that seems like something in the scripts that needs looking into, or something about the relationships that needs explaining.
That said, if questions on the diff are as pervasive in the remaining files as they are in the first dozen files, then I'm inclined to go with your suggestion of breaking up the PR into smaller pieces. Hundreds of comments on a single PR is going to become unmanageable, so breaking it up would speed up review times and making sure we resolve every issue more likely.
Thank you again!
|
Yeah, the more I think about it, the more it would be MUCH easier to separate this PR into multiple pieces, haha -- once I go through @ddbeck's comments, I'll be splitting this off into pieces by root folder. 😉 |
queengooborg
force-pushed
the
linter/lint-version-consistency
branch
from
fb2f5e2 to
d63653a
Compare
|
This might be useful as a guide to what's inconsistent. But I can't support merging these. The API one alone introduces numerous Chrome errors. I don't know if the linter's incorrect or what. |
|
I’ve run numerous tests on the linter’s behavior to make sure that it’s functioning as expected. As much as I’d love to say this is an easy fix, this is going to take some time to get through. There's just so much data to review in this repository. The PRs that alter the data are based on a simple concept: adjust the subfeatures to match the support status of their parents, and make sure they have been added no earlier (or removed no later) than the parent. Any invalidity it has created in the process originated from iterations of the data over time (aside from one case mentioned in #3375 (comment)). |
|
By providing smaller PRs, we might actually get a better picture here, so I think could start with reviewing everything that is not in the API folder for now and then see how we could tackle that one in smaller chunks later. Note that myself and other maintainers or staffs only have that much time and so I'm not promising to get to all of this immediately. Thanks for getting into this, though. It is a nice data quality exploration. |
|
Even still, these are going to take considerably time to get through. The number of edge cases is unknown. And in some cases a decision will need to be made as to whether the data or the test should altered. @Elchi3 What order do you want to review them in? |
|
Totally understandable -- this is a big change to the repo. Take all the time you need, neither myself nor the PRs are going anywhere. 😉 |
|
In the sub-PRs like #3375, is there a data source or is it just fixing inconsistencies by changing one piece of the data? Taking the first file, Animation.idl, it is changing from 40 to 46 for one API member. Is it just picking the earliest release and using that? |
|
The data source is within the files themselves. What each of the sub-PRs is doing is:
In doing so, it seems that we're coming across some inconsistencies in the data where the simple solution isn't the answer. |
Indeed. The inconsistency means that some data must be wrong, but it seems to me the only way to make it consistent is by finding out which is correct, which can't be derived from the data itself. Perhaps it's better to have consistent data which could be wrong, I'll leave it to other to make that call, I'm not a reviewer. |
Personally, I feel the same way about this. However, we shouldn’t ever have wrong data, which is particularly why the review is taking so long to get this PR, and its sub-PRs, merged. So far, there’s only been one special case, though quite frankly, it’s one out of thousands. @jpmedley’s got the right idea in going through and checking the PRs carefully. Sometimes it’s not the sub-features that have the inaccurate data, it’s the parent feature. It’s good that we’re checking it all now, though, rather than simply leaving it to remain inaccurate forever. |
|
"Adding a version_removed to the sub-feature if the parent feature has a
version_removed;"
I believe this is the only case where the change should be made by the
linter. Automatically change the inconsistencies concerns me greatly. The
changed PRs are large, and they take way more time than usual to review.
Meanwhile, we still need to submit PRs for enhancements, improvements, and
spec changes. This is a recipe for nasty merge conflicts.
So I'd like to propose an alternative plan with multiple stages.
- Phase 1: Flag discrepancies but DO NOT block merges based on them and
encourage contributors to fix them whenever they can. This phase should be
short (3 or 4 weeks?). It's main reason for existing is to keep other work
from being blocked and avoid the pain of nasty merge conflicts. PRs that
specifically contain discrepancy fixes should be short so that reviewers
can get through them quickly.
- Phase 2: Flag discrepancies but DO block merges based on them. This
would start after the number of discrepancies has burned down sufficiently.
Vinyl's origin PRs can give us a sense of how may we have.
In no case should the linter change the file.
Joe Medley | Technical Writer, Chrome DevRel | jmedley@google.com |
816-678-7195
*If an API's not documented it doesn't exist.*
…On Thu, Feb 7, 2019 at 12:07 PM Vinyl Darkscratch ***@***.***> wrote:
Perhaps it's better to have consistent data which could be wrong
Personally, I feel the same way about this. However, we shouldn’t ever
have wrong data, which is particularly why the review is taking so long to
get this PR, and its sub-PRs, merged. So far, there’s only been one special
case, though quite frankly, it’s one out of thousands.
@jpmedley <https://github.com/jpmedley>’s got the right idea in going
through and checking the PRs carefully. Sometimes it’s not the sub-features
that have the inaccurate data, it’s the parent feature. It’s good that
we’re checking it all now, though, rather than simply leaving it to remain
inaccurate forever.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3368 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AH0vizJcnjB7mrMoHBET5Z8smQ2HxEBiks5vLIeHgaJpZM4afU-N>
.
|
|
The linter actually doesn’t do the fixes, that’s in a separate script — and much dumber changes at that. Aside from the changes of setting any As for the plan you’re proposing...well, to be honest, that’s what I assumed we were already doing. The sub-PRs are dumb and have no source data other than itself, and as mentioned, the original data needs review already. (I just don’t have the time available to review every change, which is why I’m glad you are.) Once all the existing data is reviewed and it’s all ready to merge, that’s when this one will be ready to merge — the final piece that ensures new PRs will follow said consistency. |
|
What is your basis for changing null to false?
Joe Medley | Technical Writer, Chrome DevRel | jmedley@google.com |
816-678-7195
*If an API's not documented it doesn't exist.*
…On Thu, Feb 7, 2019 at 2:04 PM Vinyl Darkscratch ***@***.***> wrote:
The linter actually doesn’t do the fixes, that’s in a separate script —
and much dumber changes at that. Aside from the changes of setting any
null values to false, all the PR’s modifications have been manual,
particularly me going through and just doing the adjustments described up
above.
As for the plan you’re proposing...well, to be honest, that’s what I
assumed we were already doing. The sub-PRs are dumb and have no source data
other than itself, and as mentioned, the original data needs review
already. (I just don’t have the time available to review every change,
which is why I’m glad you are.) Once all the existing data is reviewed and
it’s all ready to merge, that’s when this one will be ready to merge — the
final piece that ensures new PRs will follow said consistency.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3368 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AH0vi4ljIiu6WVNO8-82KBnEzetZjnqkks5vLKLagaJpZM4afU-N>
.
|
|
ARGH! Looks like a few recent PRs created some more inconsistencies... |
|
Wouldn't it be possible to freeze the repository temporarily (i.e. not merge other PRs) once everything is consistent again? After that, all other PRs would need to be rebased anyways. Or, alternatively, couldn't this PR be merged, even if there are few remaining inconsistencies (since merging other unrebased PRs after merging a consistent state might introduce new consistencies - and break the master build - anyway)? Update: There haven't been any commits on 13-14/16/18 September, so a freeze should be feasible. |
|
Whoo, the linter passed in a local run and we've fixed all the consistency! This is ready to merge once again! |
queengooborg
removed
the
not ready
|
All inconsistencies are now fixed, so this is ready for merge again! |
There was a problem hiding this comment.
Really sorry it took me forever to take a look into this :(
The good thing is that we've been testing this for quite a few scenarios already, thanks to your PRs and keeping this branch always green.
The 3 consistency checks make sense to me and your ongoing PRs have kind of proven that they make sense also.
I think we need to try to present consistency errors a bit less cryptic to PR authors, so I have a thought about that below and then there are a few variables never used I believe (probably from an earlier version of this). Overall I believe we're close to land this as, thanks to your PRs, we've been applying the rules of this anyways, we now just need to see how to have it best in front of others :)
The code is yet again another completely different lint script, but we've been somewhat bad at re-using code and organizing our linting code to share functionality. I think it is fair to say that consolidating is a task for another day and shouldn't block this.
Thanks for your great work on this! 👍
|
Thank you very much! I'm leaning towards going for the merge -- if any of the existing PRs are failing consistency tests, then I'd say that's a sign that we'd need to give it some additional review for accuracy. |
|
Yes, or back things out or correct things otherwise. I think we would manage. Let's do this! :) |
|
Congratulations to you, @vinyldarkscratch! 🎉 |
|
Yes indeed. |
Unfortunately as time has progressed and more changes have been merged into this repository, a number of inconsistencies have been found. A huge one that occurred is a subfeature being implemented in a browser before its parent feature, if the parent feature was even implemented in the first place.
This PR is a combination from the efforts of #2463, #2875, and #3051, and also closes #1618. Kudos to @caugner for writing the first half of this PR! It offers the following consistency checks:
version_added == nullif its subfeatures are supported