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

Refactor ApiDefinitionParser to fix issue #23 #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonasseglare
Copy link

Refactor ApiDefinitionParser to fix issue #23

Description

This PR addresses the issue #23 where a compact json string is incorrectly parsed as Yaml/Raml. To address that bug, I put the logic to identify the syntax in the method guessSyntax and I also unit test that method to make sure that the bug is actually fixed. Along with this, I also restructured the code a bit in ApiDefinitionParser.

Fixes #23

Checklist

  • My contributions and commit messages follows the style guidelines of this project
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • The Pull Request has an informative and human-readable title
  • Changes are limited to a single goal (avoid scope creep)
  • Code can be automatically merged (no conflicts)
  • I confirm that I have read any Contribution guidelines (CONTRIBUTING)
  • I confirm that I wrote and/or have the right to submit the contents of my PR, by agreeing to the Developer Certificate of Origin, by adding a 'sign-off' to my commits

JsonFactory factory = new JsonFactory();
JsonGenerator generator = factory.createGenerator(output, JsonEncoding.UTF8);

try {
Copy link
Author

Choose a reason for hiding this comment

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

Do I need to keep the try-catch here or can I throw it away? Cathing it and printing the stack trace potentially hides potential bugs in my opinion.

Maybe I should keep it in this PR in order to make the changes focused.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good and clear PR. I think it should be accepted immediately. We are trying to find a good working method for contributions. I just need to get the team to verify that the bug fixes and improvements are working for everyone. Is it Ok if it takes a few more days? Agree, don't hide exceptions. Remove the silent swallowing of the exception.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, no problem, these changes are not very urgent so take your time :-) I ran across the issues that these PRs address while using the DCAT-AP-SE-Processor from another code base and though I should fix them.

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.

ApiDefinitionParser identifierar felaktigt kompakt json-data som YAML RAML
2 participants