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

refactor: simplify some cases of common.formatQueryValue #1598

Merged
merged 17 commits into from Jul 12, 2019

Conversation

laumair
Copy link
Contributor

@laumair laumair commented Jun 27, 2019

For #1404 & #1285.

This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce.

Related nock#1285
This commit simplifies array formatting by using Array.map instead of a plain for loop.
This commit adds unit test coverage for #formatQueryValue.

Related nock#1404
@paulmelnikow
Copy link
Member

@laumair Thanks so much for this contribution! It's great to have more help on #1404!

@mastermatt Would you be up for giving this a look?

Copy link
Member

@mastermatt mastermatt left a comment

Choose a reason for hiding this comment

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

thanks for this! It looks good.

I've made a couple of small comments.
Also, since there are changes to the actual logic, I think this would fall into a refactor instead of test category. But I'm still getting use to Angular's commit style, so take that with a grain of salt 😄

lib/common.js Outdated Show resolved Hide resolved
tests/test_common.js Outdated Show resolved Hide resolved
tests/test_common.js Outdated Show resolved Hide resolved
This commit replaces the usage of Object.keys() with Object.entries() to address [this](nock#1598 (comment)) comment.
This commit addresses the following comments:
1- nock#1598 (comment)
2- nock#1598 (comment)
@laumair laumair changed the title test: add tests for common.formatQueryValue refactor: add tests for common.formatQueryValue Jul 8, 2019
@laumair
Copy link
Contributor Author

laumair commented Jul 8, 2019

@mastermatt Thank you for the review.

I think this would fall into a refactor instead of test category.

I believe you are referring to the PR title because the branch name already has refactor/. I've also updated the PR title.

I have also addressed your comments in 6a785a8 & 37196f1

@mastermatt
Copy link
Member

Travis is failing because of Prettier.
npm run prettier

@laumair
Copy link
Contributor Author

laumair commented Jul 8, 2019

Travis is failing because of Prettier.
npm run prettier

@mastermatt Fixed with 6cffb89.

@mastermatt
Copy link
Member

The changes in lib look good to me.

I hesitate on approving the PR in general because of the tests and this comment #1588 (comment)

@paulmelnikow @gr2m Should these tests be rewritten in terms of calls to http?

This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce.

Related nock#1285
This commit simplifies array formatting by using Array.map instead of a plain for loop.
This commit adds unit test coverage for #formatQueryValue.

Related nock#1404
This commit replaces the usage of Object.keys() with Object.entries() to address [this](nock#1598 (comment)) comment.
This commit addresses the following comments:
1- nock#1598 (comment)
2- nock#1598 (comment)
@gr2m gr2m force-pushed the refactor/formatQueryValue branch from 6cffb89 to 77ac326 Compare July 8, 2019 23:39
@gr2m
Copy link
Member

gr2m commented Jul 8, 2019

I’ve rebased on latest master. Looking at the changes right now

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Refactoring looks good to me 👍

I don’t think the tests add any more coverage though compared to latest beta?

@mastermatt
Copy link
Member

@gr2m is your recommendation to remove the new tests in this PR and merge just the lib/common.js refactor?

@gr2m
Copy link
Member

gr2m commented Jul 10, 2019

Yes, since the tests are only unit tests any way, which could end up preventing us from removing dead code in the future

@laumair laumair changed the title refactor: add tests for common.formatQueryValue refactor: simplify some cases of common.formatQueryValue Jul 11, 2019
@laumair
Copy link
Contributor Author

laumair commented Jul 11, 2019

@gr2m @mastermatt Done! I have removed the unit tests.

@gr2m gr2m merged commit 1e0acad into nock:beta Jul 12, 2019
@laumair laumair deleted the refactor/formatQueryValue branch July 13, 2019 05:23
@nockbot
Copy link
Collaborator

nockbot commented Jul 15, 2019

🎉 This PR is included in version 11.0.0-beta.24 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nockbot
Copy link
Collaborator

nockbot commented Aug 12, 2019

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

gr2m pushed a commit that referenced this pull request Sep 4, 2019
* refactor(common): replace lodash.forOwn with Array.reduce

This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce.

Related #1285

* refactor(commong): use Array.map

This commit simplifies array formatting by using Array.map instead of a plain for loop.

* test(common): add unit test coverage for #formatQueryValue

This commit adds unit test coverage for #formatQueryValue.

Related #1404

* chore: run prettier to fix lint errors

* fix: use Object.entries() in favor of Object.keys()

This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment.

* test(common): break down tests

This commit addresses the following comments:
1- #1598 (comment)
2- #1598 (comment)

* chore: run prettier to fix lint errors

* refactor(common): replace lodash.forOwn with Array.reduce

This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce.

Related #1285

* refactor(commong): use Array.map

This commit simplifies array formatting by using Array.map instead of a plain for loop.

* test(common): add unit test coverage for #formatQueryValue

This commit adds unit test coverage for #formatQueryValue.

Related #1404

* chore: run prettier to fix lint errors

* fix: use Object.entries() in favor of Object.keys()

This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment.

* test(common): break down tests

This commit addresses the following comments:
1- #1598 (comment)
2- #1598 (comment)

* chore: run prettier to fix lint errors

* chore: remove unnecessary formatting changes
gr2m pushed a commit that referenced this pull request Sep 4, 2019
* refactor(common): replace lodash.forOwn with Array.reduce

This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce.

Related #1285

* refactor(commong): use Array.map

This commit simplifies array formatting by using Array.map instead of a plain for loop.

* test(common): add unit test coverage for #formatQueryValue

This commit adds unit test coverage for #formatQueryValue.

Related #1404

* chore: run prettier to fix lint errors

* fix: use Object.entries() in favor of Object.keys()

This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment.

* test(common): break down tests

This commit addresses the following comments:
1- #1598 (comment)
2- #1598 (comment)

* chore: run prettier to fix lint errors

* refactor(common): replace lodash.forOwn with Array.reduce

This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce.

Related #1285

* refactor(commong): use Array.map

This commit simplifies array formatting by using Array.map instead of a plain for loop.

* test(common): add unit test coverage for #formatQueryValue

This commit adds unit test coverage for #formatQueryValue.

Related #1404

* chore: run prettier to fix lint errors

* fix: use Object.entries() in favor of Object.keys()

This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment.

* test(common): break down tests

This commit addresses the following comments:
1- #1598 (comment)
2- #1598 (comment)

* chore: run prettier to fix lint errors

* chore: remove unnecessary formatting changes
gr2m pushed a commit that referenced this pull request Sep 5, 2019
* refactor(common): replace lodash.forOwn with Array.reduce

This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce.

Related #1285

* refactor(commong): use Array.map

This commit simplifies array formatting by using Array.map instead of a plain for loop.

* test(common): add unit test coverage for #formatQueryValue

This commit adds unit test coverage for #formatQueryValue.

Related #1404

* chore: run prettier to fix lint errors

* fix: use Object.entries() in favor of Object.keys()

This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment.

* test(common): break down tests

This commit addresses the following comments:
1- #1598 (comment)
2- #1598 (comment)

* chore: run prettier to fix lint errors

* refactor(common): replace lodash.forOwn with Array.reduce

This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce.

Related #1285

* refactor(commong): use Array.map

This commit simplifies array formatting by using Array.map instead of a plain for loop.

* test(common): add unit test coverage for #formatQueryValue

This commit adds unit test coverage for #formatQueryValue.

Related #1404

* chore: run prettier to fix lint errors

* fix: use Object.entries() in favor of Object.keys()

This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment.

* test(common): break down tests

This commit addresses the following comments:
1- #1598 (comment)
2- #1598 (comment)

* chore: run prettier to fix lint errors

* chore: remove unnecessary formatting changes
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.

None yet

5 participants