-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Revert "Add check for delete expression must be optional" #38154
Conversation
This reverts commit 39beb1d.
I actually think this is a good change and it did catch some potential errors in the VS Code codebase. For a codebase of VS Code's size, 12 errors isn't bad (but we also pretty rarely use My only complaint is that it took me a a minute or so to understand what the error message meant. Once I did though, the error was obvious |
I think that's fine (the break will still happen in a subsequent version) but the message that we communicate is that users shouldn't have to worry about breaks between the beta and the RC. That's not always true (accidents happen) but this seems easy to prevent. |
Rather than apply this PR, cut the branch tomorrow afternoon, and then land a revert of this revert tomorrow afternoon, why not just re-target this PR to the 3.9 branch tomorrow afternoon? |
That's a really good idea, I'll do that. Thanks! |
(Though while this is a "break" it definitely falls into the bucket of "helpful new errors that point out something most definitely done incorrectly", and we only generally avoid extra breaks between beta and rc (there's a much stronger argument between rc and release, where I'd definitely say it shouldn't), and it's, at least internally, done more for code stability - this change is minor, so that's not a concern. I would at least bring this up in the design meeting - this might be something we'd rather land sooner rather than later, since the design for this error has been done for 3 years already. The issue simply was never re-triaged correctly, so it fell through the cracks until @Kingwl picked it up!) |
I get that, but it seems easy enough to hold off on for 4.0 to not add too much disruption to the RC. Let's chat about it tomorrow. Side note: reading it again, I want to mention that my original comment in the other issue clearly comes off as curt and frustrated, but I should have been more understanding and thoughtful, so sorry about that. |
Actually a double revert will be easier for me to be honest |
Reverts #37921, given feedback on microsoft/vscode#96022