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

https://github.com/snyk/snyk/issues/225 #226

Closed
wants to merge 2 commits into from
Closed

https://github.com/snyk/snyk/issues/225 #226

wants to merge 2 commits into from

Conversation

brownjava
Copy link

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Fixes https://github.com/snyk/snyk/issues/225

Where should the reviewer start?

package.json

How should this be manually tested?

Run "npm pack"

Any background context you want to provide?

Background should be in the ticket.

What are the relevant tickets?

https://github.com/snyk/snyk/issues/225

@CLAassistant
Copy link

CLAassistant commented Sep 26, 2018

CLA assistant check
All committers have signed the CLA.

@miiila
Copy link
Contributor

miiila commented Sep 27, 2018

Hello @brownjava, thank you very much for catching this and for suggesting a fix. It would definitely solve the issue, but we would like to use this opportunity and improve this part of the codebase.

If you would like to adjust your PR, my suggestions would be:

If you don't have a capacity or time to adjust, don't worry, we will handle this internally in the next 2 weeks.

Thank you again for your help, we really appreciate it.

@brownjava
Copy link
Author

OK! I've submitted a new version changed based on feedback.

@Kirill89
Copy link
Contributor

Kirill89 commented Oct 3, 2018

@brownjava thank you for your help! I decided to move thouse constants to json file instead of txt. So, new PR created at #230

@brownjava
Copy link
Author

Sounds good! I'll close this PR, then.

@brownjava brownjava closed this Oct 3, 2018
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.

4 participants