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

Compare strings with strict equality operators #2582

Closed

Conversation

thefourtheye
Copy link
Contributor

@seishun
Copy link
Contributor

seishun commented Aug 27, 2015

Why? If it doesn't change the behavior, then it's pointless code churn. If it does, then it's a breaking change.

This patch makes sure that the strings are compared with strict
equality operator.
This patch makes sure that the strings are compared with strict
equality operator.
This patch makes sure that the strings are compared with strict
equality operators.
This patch makes sure that the strings are compared with strict
equality operators.
@rvagg
Copy link
Member

rvagg commented Aug 28, 2015

agreed, I'm somewhere between -1 and -0 on this, unless there's a measurable performance gain from doing this then I'm not a fan, sorry for the second negative comment in a row @thefourtheye

@thefourtheye
Copy link
Contributor Author

@seishun @rvagg No problem :-)

@ronkorving
Copy link
Contributor

lgtm, it's one step in the direction of unification (style, if nothing else).

@seishun
Copy link
Contributor

seishun commented Aug 28, 2015

I'm not convinced that the majority agrees on the "use === unconditionally everywhere" style. Either way, it seems very unlikely it will ever be enforced throughout the code base anyway, so why apply it partially?

@pmq20
Copy link
Contributor

pmq20 commented Aug 28, 2015

@seishun if it were a breaking change then it would be picked up by the CI. And I think the code churn is not pointless, because == i.e. comparison w/ type conversions could easily introduce bugs and therefore should be avoid.

@ronkorving
Copy link
Contributor

Also, he applied it partially because last time he did it not-so-partially and got bad feedback on that.

@rvagg
Copy link
Member

rvagg commented Aug 28, 2015

@seishun if it were a breaking change then it would be picked up by the CI.

Not true, we don't have 100% coverage and we certainly don't test anything near the number of weird ways people use Node and are expecting it to behave. We have a diverse ecosystem and are always running in to edge-cases and subtle bugs encountered by novel uses.

@pmq20
Copy link
Contributor

pmq20 commented Aug 28, 2015

@rvagg Agreed with you. 👎 on the PR then.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 28, 2015

Also -1. Just make changes like this when you're modifying the code for more meaningful reasons.

@Fishrock123
Copy link
Contributor

Yeah... it's hard to agree to change these once they are there. We should just make sure we always land with strict equality, unless explicitly sating why otherwise.

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.

7 participants