Skip to content
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: validation error when ! is in the path #6754

Merged
merged 5 commits into from
Mar 21, 2018
Merged

Conversation

byzyk
Copy link
Member

@byzyk byzyk commented Mar 14, 2018

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

yes

If relevant, link to documentation update:

Summary

Throw validation error when ! was found in a path.
Closes #6742, closes #5320.

Does this PR introduce a breaking change?

no

Other information

@byzyk
Copy link
Member Author

byzyk commented Mar 14, 2018

Hi @sokra, I fixed regexp inside absolute path validator to be able to check if there is ! present in a path.

However, I can't test my changes because it seems no matter what I do in schemas/ajv.absolutePath.js it doesn't reflect. For example, I tried to change the error message but it had no effect. It's not the package linking problem I checked that a couple of times already.

Please advice are there any extra steps required to make this work or am I missing something?

@chengluyu
Copy link

I think you should add a new message when ! is included in absolute path. Here's my solution.

p.s. I wonder why people don't use path.isAbsolute.

if(!passes) {
callback.errors = [getErrorFor(expected, data, schema)];

module.exports = ajv =>
Copy link
Member

Choose a reason for hiding this comment

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

Why code format changed?

Copy link
Member

Choose a reason for hiding this comment

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

@montogeek prettier

Copy link
Member

Choose a reason for hiding this comment

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

I understand that src code was already formatted with prettier, he didn't changed anything to increment line width

Copy link
Member

Choose a reason for hiding this comment

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

This file never was reformatted by prettier.

Copy link
Member

Choose a reason for hiding this comment

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

looks like the schemas folder is not included in the prettier scripts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the file was never formatted, when I first opened it there were 19 ESLint violations.

Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

Create a separate message explaining that ! should not be used and why when it's used.

It should disallow ! independent of the expected value.

@webpack-bot
Copy link
Contributor

@byzyk Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@byzyk
Copy link
Member Author

byzyk commented Mar 18, 2018

hey @sokra, I resolved some conflicts, does it look ok to be merged now?

Also, what would be the right place for docs update? Should we add a warning in all related places (eg. output.path, output.filename)?

This was referenced Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants