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

chore: don't disable rule eqeqeq #1978

Merged
merged 7 commits into from
Mar 2, 2021
Merged

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Mar 1, 2021

@legendecas
Copy link
Member

the "smart" option would not complain on maybeNullOrUndefined == null. I had been using this pattern previously, however after the discussion several members of the project came to a conclusion that this pattern has to be avoided and we should stick to "eqeqeq". So, is the "smart" option desired in this context?

@Flarna
Copy link
Member Author

Flarna commented Mar 1, 2021

@legendecas Does this mean we should always write (foo !== null) && (foo !== undefined)?
I wonder if this holds still true as there are quite a lot places meanwhile where the nullish coalescing operator ?? is used or optional chaining which both aim to vanish the difference between null/undefined.

I think that making a difference between null/undefined is nearly always a source of bugs and personally I would prefer a lint rule which even enforces to use != null instead !== null. But this PR only aims to enable the rule and stay in sync with API.

@dyladan
Copy link
Member

dyladan commented Mar 1, 2021

I prefer to use != foo because it is terser than (foo !== null) && (foo !== undefined), but it seems most developers these days are used to always use eqeqeq. When there is a eqeq in a codebase it is surprising to some people and they don't always immediately notice or know the reason for its use. For that reason, I think using eqeqeq in a project where there are many contributors of various backgrounds is a practical choice.

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #1978 (575f4d6) into main (51071cd) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1978      +/-   ##
==========================================
+ Coverage   92.89%   93.01%   +0.12%     
==========================================
  Files         152      150       -2     
  Lines        5922     5838      -84     
  Branches     1245     1227      -18     
==========================================
- Hits         5501     5430      -71     
+ Misses        421      408      -13     
Impacted Files Coverage Δ
...emetry-core/src/baggage/propagation/HttpBaggage.ts 98.24% <100.00%> (+0.03%) ⬆️
...ckages/opentelemetry-core/src/utils/environment.ts 100.00% <100.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 99.23% <100.00%> (ø)
...ntelemetry-propagator-b3/src/B3SinglePropagator.ts 100.00% <100.00%> (ø)
...es/opentelemetry-context-zone-peer-dep/src/util.ts
...ry-context-zone-peer-dep/src/ZoneContextManager.ts
...kages/opentelemetry-node/src/NodeTracerProvider.ts 100.00% <0.00%> (+5.88%) ⬆️

@dyladan
Copy link
Member

dyladan commented Mar 1, 2021

I'm fine either way, we just need to make a decision and stick to it

@obecny
Copy link
Member

obecny commented Mar 1, 2021

Nice catch @legendecas , I would vote then for removing smart to avoid more confusions. It will still be fine as

for anything else than number, string or boolean

if (!foo)
if (foo)
instead of 
(foo !== null) && (foo !== undefined)

for number

if (typeof a === 'number')
if (typeof a !== 'number')

for string

if (typeof a === 'string')
if (typeof a !== 'string')

for boolean

if (typeof a === 'boolean')
if (typeof a !== 'boolean')

so in any case we won't have this

(foo !== null) && (foo !== undefined)

@Flarna
Copy link
Member Author

Flarna commented Mar 1, 2021

@obecny I agree if we know the type but what about cases where we don't know the type like here?

@obecny
Copy link
Member

obecny commented Mar 1, 2021

@obecny I agree if we know the type but what about cases where we don't know the type like here?

honestly I don't recall such cases, if you are checking for something you always know what you want to have and you can simply choose the appropriate way of checking that.

@Flarna
Copy link
Member Author

Flarna commented Mar 1, 2021

update to use strict eqeqeq. I'm not that happy with the result as I'm not able to avoid the separate null/undefined checks but as most people prefer strict checks lets go for it.

Update: As said I don't like the stricter variant therefore I reverted this. I think it's best to go with the smart variant for now like in API - or keep the rule completely disabled like it was in the past.

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

Successfully merging this pull request may close these issues.

5 participants