Opening for extension #189
Conversation
|
Be aware that the API is not officially supported. This tool is a CLI-tool in the first place. When changes are made and the API is affected by them, it will not be reflected in the semantic versioning. If you intend to use the API it is on your own risk. However, I don't mind having this as an extension, but you will have to add automated tests and best with one or more meaningful use-cases, before it will be merged. Also, consequently the new properties should be available on all |
|
I have enabled the API extension for all methods of public API. Added some simple tests. |
|
Hi @codekie. I have rebased it on top of my previous contribution. Please let me know what you think. |
There was a problem hiding this comment.
LGTM in general. Maybe consider to support the consumption of an array of post-processors (instead of just one) where the result of the predecessor passes the result as input to the next post-processor so that they can be chained. Would that make sense? What do you think?
|
|
||
| // TEST SUITES | ||
|
|
||
| describe('Main API', function() { |
There was a problem hiding this comment.
Do these test-cases test if it successfully uses the custom factory? Won't the validation-results here be the same if omit the custom factory?
There was a problem hiding this comment.
yes it would. My point in this test was to show that the injection mechanism works as expected. In other words that injecting Ajv from outside with no modification you should expect the same results as if you would use the build in one.
There was a problem hiding this comment.
If you want I can add a test showing using different Ajv instance. Frankly thats my primary use case. I want to detect additional properties even if you use composed schemas. So for that I can use unevaluatedProperties that is supported by Ajv 2019. So it work in combo. In postprocessor I am adding unevaluateProperties flag and I am using Ajv 2019.
Another use case would be to configure Ajv instance with custom validations (https://ajv.js.org/guide/user-keywords.html)
There was a problem hiding this comment.
It would be good to have at least one test case that tests a different configuration. Just to have that covered as well.
Hi, it does make sense. But can be easily achieved by chaining functions externally. I have modified the test code to illustrate that. Maybe lets keep it as is for now? |
|
@codekie could you let me know what do you think about this MR ? what is missing / unclear ? |
|
@bartoszm sorry for the slow responses, but I've been very busy with ongoing projects the past weeks. I hope that I'll have time to have a closer look at this, this week. I'll address you if I have questions. |
No worries, I've been there many times :) |
Two extension points added:
Fixes #188