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

Feature request: Ajv formats addFormat support #60

Closed
cfuerst opened this issue Apr 17, 2024 · 9 comments
Closed

Feature request: Ajv formats addFormat support #60

cfuerst opened this issue Apr 17, 2024 · 9 comments
Labels
enhancement New feature or request

Comments

@cfuerst
Copy link
Contributor

cfuerst commented Apr 17, 2024

Hi There 👋 first of all thank you for this great GitHub action ❤️

I need to do exactly what you supporting here with the addition that I have custom formats in my schema.

For example:

// in the validator itself:
ajv.addFormat("cron_expression", /(((\d+,)+\d+|(\d+(\/|-)\d+)|\d+|\*) ?){5,7}/)

// json
{
  "some_property_name": { "type": "string", "format": "cron_expression" }
}

Could (and would you be open to) you imagine a smart and generic way how this action could support custom formats?
If so I would be happy to try to drive a PR regarding that.

Cheers

@GrantBirki
Copy link
Owner

@cfuerst I would absolutely support this! Please do open a PR and I'll take a look and try to answer any questions you may have 🎉

@GrantBirki GrantBirki added the enhancement New feature or request label Apr 17, 2024
@cfuerst
Copy link
Contributor Author

cfuerst commented Apr 17, 2024

What do you think of going with an additional parameter ajv_custom_regexp_formats which is handled same as files option with getMultilineInput() which would look something like this:

      - name: json-yaml-validate
        uses: GrantBirki/json-yaml-validate@vX.X.X
        with:
            ajv_custom_regexp_formats: |
                cron_expression=/(((\d+,)+\d+|(\d+(\/|-)\d+)|\d+|\*) ?){5,7}/
                another_custom_format=/[A-z]+/

It's not pretty to pass a key value pair like this though. Maybe you have a better idea.

setting any value in ajv_custom_regexp_formats would also implicitly set use_ajv_formats to true.
-> addFormat() seems to be part of ajv core

Feedback greatly appreciated 🙇

Edit: for the sake of simplicity I would only support regexp out of the signature of addFormat

@cfuerst
Copy link
Contributor Author

cfuerst commented Apr 17, 2024

@GrantBirki I added a PR for this #61 let me know what you think about it

@GrantBirki
Copy link
Owner

@cfuerst thank you so much for this feature and being super speedy ⚡!

I have published a new release v2.7.0 which contains your new feature. Please give it a go and let me know if it all works okay for you.

If it does work, I'll update the main release v2 of this Action to pick up your changes there as well.

Thank you! 🙇

@cfuerst
Copy link
Contributor Author

cfuerst commented Apr 17, 2024

Awesome thank you ill try it out tomorrow asap and give you feedback.

You had quite a quick merge finger here hehe, there are still 2 tests i have not implemented yet which would cover the non happy paths (not sure if they are even needed) - see the last 2 tests from json validator test. Sorry for that, but i can take care of them too. Whats your opinion how to deal with those cases?

@GrantBirki
Copy link
Owner

I was indeed too speedy! If you want to open another PR to fill in those tests cases, that would be great! I don't have any preference as to how they are done. Even without them, I think its working great and that's the reason for acceptance tests too 😃

@cfuerst
Copy link
Contributor Author

cfuerst commented Apr 18, 2024

#64, ill shortly try out v2.7.0 in the meanwhile

@cfuerst
Copy link
Contributor Author

cfuerst commented Apr 18, 2024

👍 v2.7.0 works like a charm in my repo

@GrantBirki
Copy link
Owner

Merged #64 🎉

New release published v2.7.1 and I have updated the main tag of v2 to point to v2.7.1 via this workflow so we should be all set!

Thank you so much for your great contributions here 🙇 ⭐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants