Skip to content

Conversation

@bryceosterhaus
Copy link
Member

fixes #14

@izaera, could you check my changes to make sure I didn't break anything? I updated all cases of someVal == null and someVal != null to also include a check for undefined since we are enforcing ===

@bryceosterhaus
Copy link
Member Author

fyi, 400+ breaks of eqeqeq on DXP

cleanDirectory(tempPath);

if (initCwd != null) {
if (initCwd !== null && initCwd !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we express this as if (initCwd) . I find if (initCwd !== null && initCwd !== undefined) a bit too complex to use it in every place of the code...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal with using initCwd !== null && initCwd !== undefined is that it is safer for a wide refactor since initCwd != null is equal to initCwd !== null && initCwd !== undefined

If I was writing new code, I would likely use if (initCwd), but I can't totally be sure that this bit of code would work with that unless I tested it thoroughly(which I didn't since I touched lots of code).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is only for this PR, it's OK and we may refactor them when we come back to the code. But if it is going to be our usual pattern, I find it a bit too much for my refined taste

Just saw this from below. Yep, this is just for this PR and not something I intend for us to do. Only made that change for some safety in refactoring.

}

if (!subscriptionKey || subscriptionKey == '') {
if (!subscriptionKey) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return getNamespace(moduleName) != null;
const nameSpace = getNamespace(moduleName);

return nameSpace !== null && nameSpace !== undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be !!getNamespace(moduleName) as well...

@izaera
Copy link
Member

izaera commented Oct 21, 2021

AEIOK (apparently, everything is OK).

I'm a bit worried about those if (x === null || x === undefined) constructs. If it is only for this PR, it's OK and we may refactor them when we come back to the code. But if it is going to be our usual pattern, I find it a bit too much for my refined taste :bowtie:

@kresimir-coko
Copy link
Member

Ah, pretty cool rule, didn't know it was present in the default ESLint config, but it makes sense that it is. It's gonna be awesome to enforce these.

Oh, also LNHTM

@bryceosterhaus
Copy link
Member Author

Looks like we are on the same page and gonna merge.

@bryceosterhaus bryceosterhaus merged commit b2fc9e4 into liferay:master Oct 21, 2021
@bryceosterhaus bryceosterhaus deleted the 14 branch October 21, 2021 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce use of "strict equality"

3 participants