Skip to content

throw error if nothing to inject#249

Open
tiberiuzuld wants to merge 1 commit intoklei:masterfrom
tiberiuzuld:throwErrorIfEmpty
Open

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

Conversation

@tiberiuzuld
Copy link

@tiberiuzuld tiberiuzuld commented Nov 21, 2018

No description provided.

@rejas rejas requested a review from joakimbeng November 23, 2018 07:57
joakimbeng
joakimbeng requested changes Dec 5, 2018
View reviewed changes
Copy link
Member

@joakimbeng joakimbeng left a comment

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)

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
Member

@joakimbeng joakimbeng Dec 5, 2018

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
Author

@tiberiuzuld tiberiuzuld Dec 9, 2018

Choose a reason for hiding this comment

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

Done

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
Member

@joakimbeng joakimbeng Dec 5, 2018

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
Author

@tiberiuzuld tiberiuzuld Dec 9, 2018

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
Copy link
Author

tiberiuzuld commented Dec 6, 2018 •
edited
Loading

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
Author

tiberiuzuld commented Dec 9, 2018

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

buberdds commented Aug 28, 2020

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

Reviewers

@joakimbeng joakimbeng joakimbeng requested changes

Assignees

@tiberiuzuld tiberiuzuld

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tiberiuzuld @buberdds @joakimbeng