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 - Consolidating duplicate code. Minor bug fixes #73

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

3choBoomer
Copy link

@3choBoomer 3choBoomer commented Jul 24, 2018

I started using magic-chunks recently and after looking through the code, I wanted to contribute a little.

I saw a decent amount of duplicated code blocks and some minor issues with grammar and parameter order that I wanted to resolve.

All tests pass with only two minor changes to the tests themselves. One of them was to eliminate the hiding of an error due to the order that the parameters were being checked. (see inline note).

Please review and provide feedback.

Thanks for making this tool, it's quite helpful!

var document = new YamlDocument(@"a:
x: 1
b: 2
c: 3");
Copy link
Author

Choose a reason for hiding this comment

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

I moved the check for the lack of a root object from the call to ReplaceKey to the constructor since it could be checked with no other data required.

Because of this, the constructor now throws the Root Element Not Present error.

This test was modified to only validate the empty path exception, and a new test was added below to verify that the no root element exception is thrown by the constructor.

@3choBoomer
Copy link
Author

Looks like npm is throwing a few minor security warnings and that is breaking the build:

Cake.exe : npm WARN notice [SECURITY] lodash has the following vulnerability: 1 low. Go here for more details: https://nodesecurity.io/advisories?search=lodash&version=3.10.1 - Run npm i npm@latest -g to upgrade your npm
version, and then npm audit to get more info.
At line:1 char:1
& "C:\projects\magic-chunks\build\tools\Cake\Cake.exe" "build.cake" - ...

    CategoryInfo          : NotSpecified: (npm WARN notice... get more info.:String) [], RemoteException
   FullyQualifiedErrorId : NativeCommandError

npm
WARN notice [SECURITY] sync-exec has the following vulnerability: 1 moderate. Go here for more details: https://nodesecurity.io/advisories?search=sync-exec&version=0.6.2 - Run `npm i npm@latest -g` to upgrade your npm version, 
and then `npm audit` to get 
more info.

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.

1 participant