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

Returning error messages from applicators #52

Merged
merged 6 commits into from
Sep 8, 2023

Conversation

EmilsOzolins
Copy link
Contributor

Work on #51 issue

@harrel56
Copy link
Owner

harrel56 commented Sep 6, 2023

Funny stuff - GH doesn't make repo variables accessible in PRs from forks. I thought it's only the case for secrets :) Did a quick research how to work around this issue and it's not so straight forward - probably the easiest ad-hoc solution would be if you added required variables to your fork (see attached screenshot).

There still is a SonarCloud step which uses secrets - might disable it for fork PRs temporarily until I figure out how to do it correctly.
image

@harrel56
Copy link
Owner

harrel56 commented Sep 6, 2023

BTW thanks for the willingness to contribute :) Let me know if you want to just take care of oneOf applicator or all of them. Because there probably will be some decisions to make:

  • some applicators maybe don't need to produce any error message
  • exact content of these new error messages
  • I'm starting to feel that Applicator interface might be up for removal

@EmilsOzolins EmilsOzolins marked this pull request as ready for review September 7, 2023 18:57
@EmilsOzolins
Copy link
Contributor Author

Funny stuff - GH doesn't make repo variables accessible in PRs from forks. I thought it's only the case for secrets :) Did a quick research how to work around this issue and it's not so straight forward - probably the easiest ad-hoc solution would be if you added required variables to your fork (see attached screenshot).

There still is a SonarCloud step which uses secrets - might disable it for fork PRs temporarily until I figure out how to do it correctly. image

Thanks, I did exactly as you suggested. Let's see if it works now :)

@EmilsOzolins
Copy link
Contributor Author

BTW thanks for the willingness to contribute :) Let me know if you want to just take care of oneOf applicator or all of them. Because there probably will be some decisions to make:

  • some applicators maybe don't need to produce any error message
  • exact content of these new error messages
  • I'm starting to feel that Applicator interface might be up for removal

I can take care of all of them but as I am not closely familiar with all of the specification, I only handled anyOf, allOf, not and oneOf for now. Also removed the Applicators interface as it seemed a bit redundant, as you already mentioned.

Let me know what you think about current proposal and if there is any other applicator I should take care of :)

@EmilsOzolins EmilsOzolins changed the title [WIP] Returning error messages from applicators Returning error messages from applicators Sep 7, 2023
Copy link
Owner

@harrel56 harrel56 left a comment

Choose a reason for hiding this comment

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

This looks really good - left a few comments, mainly error messages nitpicks. Still not 100% about the suggestions but I would at least try to be more consistent

src/main/java/dev/harrel/jsonschema/Applicators.java Outdated Show resolved Hide resolved
src/main/java/dev/harrel/jsonschema/Applicators.java Outdated Show resolved Hide resolved
src/main/java/dev/harrel/jsonschema/Applicators.java Outdated Show resolved Hide resolved
src/main/java/dev/harrel/jsonschema/Applicators.java Outdated Show resolved Hide resolved
src/main/java/dev/harrel/jsonschema/Applicators.java Outdated Show resolved Hide resolved
src/main/java/dev/harrel/jsonschema/Applicators.java Outdated Show resolved Hide resolved
@harrel56
Copy link
Owner

harrel56 commented Sep 7, 2023

As for the failing workflows - I think I'll just test it out locally and merge. I'll try to fix it later on, so contribution would be smoother.

@harrel56
Copy link
Owner

harrel56 commented Sep 7, 2023

Yeah, and I think the scope for now is fine. Gonna take care of whatever is left later on. Hopefully tomorrow all the changes could be released

@harrel56 harrel56 linked an issue Sep 7, 2023 that may be closed by this pull request
@EmilsOzolins
Copy link
Contributor Author

EmilsOzolins commented Sep 8, 2023

Thanks for the comments. I completely agree with your suggestions and have updated it accordingly

Copy link
Owner

@harrel56 harrel56 left a comment

Choose a reason for hiding this comment

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

Looking good - appreciate your help :) gonna merge now

@harrel56 harrel56 merged commit 9c09987 into harrel56:master Sep 8, 2023
11 checks passed
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.

Applicators don't produce errors
2 participants