Skip to content
This repository was archived by the owner on Jan 5, 2021. It is now read-only.

Conversation

bamandel
Copy link
Contributor

Added a couple methods to BomTool and some fixes to NPM and the test

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 59.841% when pulling a3a41dd on bomtool-changes into 0c11d13 on master.

Copy link
Contributor

@jamesrichard91 jamesrichard91 left a comment

Choose a reason for hiding this comment

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

runBomToolExe and runBomToolExeToFile should be in ExecutableRunner not in BomTool.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 58.299% when pulling c75ca9a on bomtool-changes into aa31fd7 on master.

@bamandel bamandel dismissed jamesrichard91’s stale review July 10, 2017 20:23

Removed problem methods

Copy link
Contributor

@JakeMathews JakeMathews left a comment

Choose a reason for hiding this comment

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

Shouldn't this logic be put in ExecutableRunner? Even with your 'Cleaned up BomTool' commit. I agree with James

@bamandel
Copy link
Contributor Author

Well I removed the method from the parent BomTool class and am just using it as a helper method in my local NPMBomTool. I don't see how it would work better in the actual ExecutableRunner

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 57.667% when pulling c3bbffb on bomtool-changes into ff47b45 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 57.736% when pulling 2906181 on bomtool-changes into ff47b45 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 57.736% when pulling 8817423 on bomtool-changes into ff47b45 on master.

@bamandel bamandel merged commit 2f7d4ba into master Jul 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants