-
Notifications
You must be signed in to change notification settings - Fork 2.3k
chore(driverProviders): add warnings to extra driver provider parameters #3873
Conversation
437462f
to
a317b84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some nits
warn += 'local' !== providerType && config.seleniumServerJar ? 'seleniumServerJar, ' : ''; | ||
warn += 'mock' !== providerType && config.mockSelenium ? 'mockSelenium, ' : ''; | ||
if (warn !== '') { | ||
logger.warn(warnInto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two will be on separate lines; is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could make them a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
let warnInto = 'Using driver provider ' + providerType + ', but also found extra'; | ||
let warn = ''; | ||
warn += 'direct' !== providerType && config.directConnect ? 'directConnect: ' : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be 'directConnect, '
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The driver provider is direct. Probably should call it directConnect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I meant at the end: you have 'directConnect: '
but it should be 'directConnect, '
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Fixed.
export let logWarnings = (providerType: string, config: Config): void => { | ||
|
||
let warnInto = 'Using driver provider ' + providerType + ', but also found extra'; | ||
let warn = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make more sense to have this be a string[]
, and then do a join(', ')
at the end. This way you won't get an extra comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I thought about it after I wrote it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
8050720
to
8364545
Compare
- builds the driver provider in lib/driverProviders/index instead of lib/runner closes angular#1945
8364545
to
c49c6c6
Compare
closes #1945