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

Fix not specified array as primary option for "function-url-scheme-whitelist" #2447

Merged
merged 1 commit into from Mar 31, 2017

Conversation

4 participants
@hudochenkov
Member

hudochenkov commented Mar 27, 2017

Which issue, if any, is this issue related to?

#2446

Is there anything in the PR that needs further explanation?

For some strange reason it wasn't defined, that rule can accept an array as primary option. Test didn't break, though. I wonder why? I believe we should have tests to test this behavior, but I have no idea how to do them.

I've tested results before and after this change via Node API. It stopped throwing warnings.

@jeddy3

jeddy3 approved these changes Mar 28, 2017

LGTM

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Mar 28, 2017

@jeddy3 @evilebottnawi do you know what tests to add to prevent this kind of problems in the future?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 28, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Mar 30, 2017

@hudochenkov That property ends up being used in

primaryOptionArray/*:: ?: boolean*/
. We could run try settings for tests through this function, to ensure that the settings in tests undergo the same processing they do in the real program.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 30, 2017

@davidtheclark it is be good

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Mar 31, 2017

@davidtheclark I'm afraid I didn't understand your idea :(

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Mar 31, 2017

@hudochenkov Could you merge this PR and open up an issue for follow-up, and I will try to find some time this weekend to try some things?

@hudochenkov hudochenkov merged commit 02d1697 into master Mar 31, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0006%) to 95.0%
Details

@hudochenkov hudochenkov deleted the fix-2446 branch Mar 31, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Mar 31, 2017

Added to changelog:

  • Fixed: function-url-scheme-whitelist was working incorrectly if more than one URL scheme were specified (#2447).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment