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

Remove console.info from EntityRequest error handler #210

Merged

Conversation

victorsingh
Copy link
Contributor

@victorsingh victorsingh commented Feb 13, 2018

What: Removed console.info from /data-point/packages/data-point/lib/entity-types/entity-request/resolve.js

Why: To prevent tests from being congested with logging information

How: The delete key 😄

Checklist:

  • Has Breaking changes
  • Documentation
  • Tests
  • Ready to be merged
  • Added username to all-contributors list

Edit: Edited by @paulmolluzzo

@victorsingh victorsingh changed the title Gate console.info in entity request Feature/Gate console.info in entity request Feb 13, 2018
@coveralls
Copy link

coveralls commented Feb 13, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 9c1ea95 on victorsingh:gate-console.info-in-entitiy-request into b608245 on ViacomInc:master.

@@ -129,7 +130,7 @@ function resolveRequest (acc, resolveReducer) {

// this is useful in the case the error itself is not logged by the
// implementation
console.info(redactedError.toString(), message)
if (!omitLog) console.info(redactedError.toString(), message)
Copy link
Contributor

@raingerber raingerber Feb 13, 2018

Choose a reason for hiding this comment

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

in order for this to be useful, the user would need a way of toggling showErr to be true/false. however, I think we can do what @paulmolluzzo suggested in the issue, and just remove the console.info entirely - the message is included in the error, so the users can choose to log that info if they choose

@@ -401,7 +400,6 @@ describe('resolve', () => {
return transform('request:a9', {}).catch(err => {
expect(err.statusCode).toEqual(404)
expect(err.message).toMatchSnapshot()
expect(console.info).toBeCalled()
Copy link
Contributor

Choose a reason for hiding this comment

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

we're using jest to mock console.info, so we can remove some of that code as well. please search the file for console.info and remove any code that you find if it's no longer being used.

Copy link
Contributor

@raingerber raingerber left a comment

Choose a reason for hiding this comment

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

looks good!

acatl
acatl approved these changes Feb 15, 2018
Copy link
Collaborator

@acatl acatl left a comment

Choose a reason for hiding this comment

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

@victorsingh this pr looks good but the title and description do not match the change

can u update the title/description to match the PR’s code?

@paulmolluzzo paulmolluzzo changed the title Feature/Gate console.info in entity request Remove console.info in entity request Feb 15, 2018
@paulmolluzzo
Copy link
Collaborator

@acatl I updated the title and description so we can get this merged.

Nice work @victorsingh!

@raingerber raingerber changed the title Remove console.info in entity request Remove console.info from EntityRequest error handler Feb 15, 2018
@acatl acatl merged commit a366091 into ViacomInc:master Feb 16, 2018
raingerber pushed a commit to raingerber/data-point that referenced this pull request Feb 20, 2018
Removed console.info from EntityRequest resolve error handler
raingerber pushed a commit to raingerber/data-point that referenced this pull request Feb 20, 2018
Removed console.info from EntityRequest resolve error handler
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.

5 participants