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

Using assert.strictEqual instead of equal #9950

Closed
wants to merge 1 commit into from

Conversation

codeVana
Copy link
Contributor

@codeVana codeVana commented Dec 1, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@mscdex mscdex added domain Issues and PRs related to the domain subsystem. os Issues and PRs related to the os subsystem. labels Dec 1, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

Please follow the commit message guidelines here.

@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@codeVana codeVana changed the title Codevana1 Using assert.strictEqual instead of equal Dec 1, 2016
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. Commits need to be squashed and formatted to meet commit guidelines.


assert.ok(typeof home === 'string');
assert.ok(home.indexOf(path.sep) !== -1);
assert.strictEqual(typeof home , 'string');
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the indent here, or the space before the comma.

@Trott
Copy link
Member

Trott commented Dec 21, 2016

Ping @codeVana: Are you able to make the small change requested by @cjihrig above?

@Trott
Copy link
Member

Trott commented Dec 21, 2016

Hmmm...actually, it might not even be needed once the merge conflict is done. Let me do that first and see if there's still an issue...

@Trott
Copy link
Member

Trott commented Dec 21, 2016

@cjihrig With the rebase against master, the issue you commented on is now gone. In fact, the change set for this PR is now down to just two lines. Can you confirm that it looks OK to you and update your code review appropriately?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is ✅

@Trott
Copy link
Member

Trott commented Dec 21, 2016

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 22, 2016
Use assert.strictEqual instead of assert.equal.

PR-URL: nodejs#9950
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Dec 22, 2016

Landed in dd98649.
Thanks for the contribution! 🎉

@Trott Trott closed this Dec 22, 2016
targos pushed a commit that referenced this pull request Dec 26, 2016
Use assert.strictEqual instead of assert.equal.

PR-URL: #9950
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Dec 26, 2016
Use assert.strictEqual instead of assert.equal.

PR-URL: #9950
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 27, 2016
@MylesBorins MylesBorins mentioned this pull request Dec 27, 2016
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
Use assert.strictEqual instead of assert.equal.

PR-URL: #9950
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
Use assert.strictEqual instead of assert.equal.

PR-URL: #9950
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. domain Issues and PRs related to the domain subsystem. os Issues and PRs related to the os subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants