Skip to content
This repository has been archived by the owner on May 11, 2018. It is now read-only.

Fix hasBeenWarned condition. #175

Merged
merged 2 commits into from
Feb 27, 2017
Merged

Conversation

yavorsky
Copy link
Member

@yavorsky yavorsky commented Feb 27, 2017

Remove check for static hasBeenWarned that always false.

@@ -71,7 +69,7 @@ export const validateModulesOption = (modulesOpt = "commonjs") => {

export default function normalizeOptions(opts) {
// TODO: remove whitelist in favor of include in next major
if (opts.whitelist && !hasBeenWarned) {
if (opts.whitelist) {
console.warn(
Copy link
Member

Choose a reason for hiding this comment

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

then we should add hasBeenWarned = true in the if block? otherwise it will warn multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

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

@hzoo yes! I thought it could be called once, but actually not. Updated!

@yavorsky yavorsky changed the title Remove hasBeenWarned condition. Fix hasBeenWarned condition. Feb 27, 2017
@hzoo hzoo added the i: bug label Feb 27, 2017
@hzoo hzoo merged commit 3d4baca into babel:master Feb 27, 2017
@existentialism
Copy link
Member

Should we add a test for this?

@hzoo
Copy link
Member

hzoo commented Feb 27, 2017

oh yeah, always

@hzoo
Copy link
Member

hzoo commented Feb 27, 2017

maybe bot should warn - if you change src and no test files changed 😼

@yavorsky yavorsky deleted the hasbeenwarned-cond branch February 27, 2017 15:17
@yavorsky
Copy link
Member Author

yavorsky commented Feb 27, 2017

Maybe pre-commit hook? You can't commit unless some tests were added 🙂

I'll cover it after lunch (UTC +2).

@hzoo
Copy link
Member

hzoo commented Feb 27, 2017

Nah since I don't want new contributors to not make the pr if they don't have it, can tell them after?

@yavorsky
Copy link
Member Author

I was joking :) But the bot that shows changes coverage - great idea!

@hzoo
Copy link
Member

hzoo commented Feb 27, 2017

And we do have codecov set up on babel already

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants