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

Ignore whitespace when parsing examples #278

Closed
wants to merge 2 commits into from

Conversation

hasheddan
Copy link
Contributor

Description of your changes

Updates examples parser to ignore whitespace. If a YAML document is provided that contains only whitespace then it is skipped, but does not cause an error. This is in alignment with the API types parser.

Signed-off-by: hasheddan georgedanielmangum@gmail.com

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

Testing in progress.. will be updated shortly.

Updates examples parser to ignore whitespace. If a YAML document is
provided that contains only whitespace then it is skipped, but does not
cause an error. This is in alignment with the API types parser.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Updates the examples parser to annotate the error if the ReadCloser
supports doing so.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Copy link
Contributor Author

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

These changes improve the situation, but it is still possible to encounter issues. Converting to draft temporarily.

@hasheddan hasheddan marked this pull request as draft November 10, 2022 12:15
Comment on lines +69 to +71
if isWhiteSpace(bytes) {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on adding a test case or two for the behavior you're seeing? I clearly failed to add tests in this package the first time through 😞 , but in the event we find a fix, it would be good to know if/when we accidentally regressed the changes 👍.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will add tests before moving out of draft -- I discovered this is an issue upstream as well though so will likely fix then propagate 👍🏻

@muvaf
Copy link
Contributor

muvaf commented Mar 23, 2023

@hasheddan Is this PR still relevant?

@hasheddan
Copy link
Contributor Author

@muvaf it is still relevant but is is not ready to be merged. I'll see if I can push this forward or convert to an issue 👍🏻

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

Successfully merging this pull request may close these issues.

None yet

4 participants