Skip to content

throw error if nothing to inject#249

Open
tiberiuzuld wants to merge 1 commit into
klei:masterfrom
tiberiuzuld:throwErrorIfEmpty
Open

throw error if nothing to inject#249
tiberiuzuld wants to merge 1 commit into
klei:masterfrom
tiberiuzuld:throwErrorIfEmpty

Conversation

@tiberiuzuld

Copy link
Copy Markdown

No description provided.

@rejas rejas requested a review from joakimbeng November 23, 2018 07:57

@joakimbeng joakimbeng left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change will be a breaking change, so the commit message should be something more like:

feat: throw error if nothing to inject

BREAKING CHANGE: instead of logging when nothing was injected an error will be thrown

(Side note: I guess you made this PR from the GitHub UI, or locally without running npm install first, because the git hook that validates the commit message apparently hasn't been run. Conventional Commits should be used)

Comment thread src/inject/index.js Outdated
log(cyan(filesCount) + ' file' + pluralState + ' into ' + magenta(target.relative) + '.');
} else {
log('Nothing to inject into ' + magenta(target.relative) + '.');
error('Nothing to inject into ' + magenta(target.relative) + '.');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You'll need a throw here as well, otherwise nothing will happen, it won't even log anything...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

Comment thread src/inject/index.js Outdated
log(cyan(filesCount) + ' file' + pluralState + ' into ' + magenta(target.relative) + '.');
} else {
log('Nothing to inject into ' + magenta(target.relative) + '.');
error('Nothing to inject into ' + magenta(target.relative) + '.');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also suggest adding a test for this

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added tests for both error and log if there is nothing to inject

@tiberiuzuld

tiberiuzuld commented Dec 6, 2018

Copy link
Copy Markdown
Author

Hi @joakimbeng ,
Will try to have the changes done this weekend.

What about adding an option throwErrorIfNoInject default false between log and error so it will not be a breaking change just a new feature?

And yes made the PR from GitHub UI :)

@tiberiuzuld tiberiuzuld force-pushed the throwErrorIfEmpty branch 2 times, most recently from 37ba0c8 to 294ae05 Compare December 9, 2018 15:13
@tiberiuzuld

Copy link
Copy Markdown
Author

Hi @joakimbeng ,
Added extra option throwErrorIfNoInject default false for this feature.
Added tests for log and error.
Changed the commit message to feat: throw error if nothing to inject

Please review when you have time.

Thanks

@buberdds

Copy link
Copy Markdown

Hey @joakimbeng, is there a chance to merge this PR?

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.

3 participants