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(index): support json schema format keyword again #52

Closed
wants to merge 1 commit into from
Closed

fix(index): support json schema format keyword again #52

wants to merge 1 commit into from

Conversation

Fdawgs
Copy link
Member

@Fdawgs Fdawgs commented Mar 30, 2021

closes #48

AJV v6 supported the JSON Schema format keyword however, with v7, this functionality has been moved to a separate module.

This PR simply adds that module.

Checklist

@coveralls
Copy link

coveralls commented Mar 30, 2021

Pull Request Test Coverage Report for Build 701946363

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 90.769%

Totals Coverage Status
Change from base Build 701913365: 0.1%
Covered Lines: 40
Relevant Lines: 41

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 701282640

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 90.769%

Totals Coverage Status
Change from base Build 660890763: 0.1%
Covered Lines: 40
Relevant Lines: 41

💛 - Coveralls

@climba03003
Copy link
Member

Instead of adding the support one by one. I suggest to allow user pass a custom ajv instance.

@mcollina
Copy link
Member

Can you add a note on the README about this?


I would also ask if it would make sense to bump the Ajv dep to 8.0.0 and use ajv-formats@2.0.1.

@Fdawgs
Copy link
Member Author

Fdawgs commented Mar 30, 2021

I would also ask if it would make sense to bump the Ajv dep to 8.0.0 and use ajv-formats@2.0.1.

Best to close this PR and wait for #49 then?

@mcollina
Copy link
Member

I would also ask if it would make sense to bump the Ajv dep to 8.0.0 and use ajv-formats@2.0.1.

Best to close this PR and wait for #49 then?

I just landed that

@Fdawgs
Copy link
Member Author

Fdawgs commented Mar 30, 2021

I would also ask if it would make sense to bump the Ajv dep to 8.0.0 and use ajv-formats@2.0.1.

Best to close this PR and wait for #49 then?

I just landed that

Great! 👍

Rebased and updated to 2.0.1

@mcollina
Copy link
Member

Instead of adding the support one by one. I suggest to allow user pass a custom ajv instance.

what about this @Fdawgs ?

@Fdawgs
Copy link
Member Author

Fdawgs commented Mar 30, 2021

Instead of adding the support one by one. I suggest to allow user pass a custom ajv instance.

Sounds like a separate piece of work from this fix, is it not @climba03003?

@climba03003
Copy link
Member

climba03003 commented Mar 30, 2021

Instead of adding the support one by one. I suggest to allow user pass a custom ajv instance.

Sounds like a separate piece of work from this fix, is it not @climba03003?

@Fdawgs I think it is like another solution to the issue. It pass the responsibility to the user (we only provide minimal validation, they can expend it on their needs) and increase the flexibility of validation.

@mcollina
Copy link
Member

I agree with @climba03003

@Fdawgs Fdawgs changed the title fix(index): add support for json schema formats fix(index): support json schema format keyword again Mar 30, 2021
@Fdawgs
Copy link
Member Author

Fdawgs commented Apr 11, 2021

@climba03003 @mcollina FYI got it on my todo list to come back and refactor the plugin to support this (unless someone beats me to it), so not forgotten!

@mcollina
Copy link
Member

no worries

@Fdawgs Fdawgs closed this May 12, 2021
@Fdawgs Fdawgs deleted the fix/format branch May 12, 2021 16:43
This was referenced Jun 16, 2021
@Eomm Eomm mentioned this pull request Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the support to add formats
5 participants