Skip to content
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

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Apr 24, 2020

Reverts #37921, given feedback on microsoft/vscode#96022

@mjbvz
Copy link
Contributor

mjbvz commented Apr 24, 2020

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 delete)

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

@DanielRosenwasser
Copy link
Member Author

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.

@weswigham
Copy link
Member

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?

@DanielRosenwasser
Copy link
Member Author

That's a really good idea, I'll do that. Thanks!

@weswigham
Copy link
Member

weswigham commented Apr 24, 2020

(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!)

@DanielRosenwasser
Copy link
Member Author

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.

@DanielRosenwasser
Copy link
Member Author

Actually a double revert will be easier for me to be honest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants