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

Reapply using array.isEquals instead of _primitiveArrayEquals #84017

Merged
merged 1 commit into from Nov 6, 2019

Conversation

mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Nov 6, 2019

Reverts 6193898

@mjbvz
Copy link
Contributor Author

mjbvz commented Nov 6, 2019

@alexandrudima What is the reason for reverts like, 6193898? Did they break something? Or just unexpected code motion?

I didn't create PRs for these changes as they seemed fairly uncontroversial (similar to the convert to async or use optional chaining passes people have made) and I did not know who owns many of files being effected and assumed they were orphaned/team owned.

@alexdima
Copy link
Member

alexdima commented Nov 6, 2019

1. In general..

Please, I do not want unnecessary changes in code that I maintain. Here are my personal reasons for not wanting unnecessary changes:

  1. Unnecessary changes always have a risk of breaking something for no upside (since the changes were not necessary).

  2. There is a cognitive cost that I need to pay when dealing with that code in the future. I cannot speak for others, but when I open a piece of code that I wrote, even N years ago, I "recognize" it. I "recognize" the code and (not always), the thoughts and ideas I had when writing it come back. I haven't studied this so I don't have the right concepts to describe what I am experiencing with proper names (it is not photographic memory, but maybe there is something similar that is happening in my brain). In any case, understanding a piece of code that I wrote is easier for me vs understanding a piece of code written by someone else. I don't know why this is the case, I don't know if other people are like this. If you or anyone goes in and does a little unnecessary change here, another unnecessary change there, etc. then that code will feel "foreign" to my brain and I will need to spend a whole lot more mental energy to work with it in the future. Again, I don't know if this is a general thing or only my brain works like this. I am very happy to take contributions, but I would like to take only "necessary" contributions, which have clear upsides.

2. Specifically, about this change

We can continue the debate around what is considered a "necessary" change. Perhaps you consider this change a necessary change because it improves code style (subjective), or reduces shipped code size (objective). But then I would suggest that you spend energy on improving our installer and implementing incremental updates. Shipping a solution where VS Code would update incrementally (only the changed files are downloaded) would have an objective impact of several orders of magnitudes in terms of bandwidth saved compared to eliminating one method.

I agree this specific change does not have the additional downside of a loss of perf as I described here for the other changes I reverted.

I can accept this change because I see that it means a lot to you. But please, I do not want unnecessary changes in the future. I want contributions that save me time, and this specific contribution has already cost me 1 hour to write this reply (did I mention I am a very slow writer) :).

@alexdima alexdima merged commit 6d39826 into microsoft:master Nov 6, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants