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

Validate content type strings in validateDoc #471

Conversation

PARAIPAN9
Copy link
Contributor

@PARAIPAN9 PARAIPAN9 commented Dec 11, 2023

Motivation

Modifications

  • Create extracting and validation functions for content type strings.

Result

  • This will help catch invalid content types before code generation.

Test Plan

  • Test it on an openapi document to make sure the errors are thrown as intended and wrote additional unit tests.

@simonjbeaumont
Copy link
Collaborator

Thanks for your continued efforts @PARAIPAN9 ❤️. We'll take a look at this, but just to manage your expectations, we'll likely want to merge this after we ship 1.0.0 (hopefully on Wednesday). This could well be the first post-1.0 PR though :)

@simonjbeaumont
Copy link
Collaborator

@swift-server-bot test this please

@simonjbeaumont simonjbeaumont added the 🔨 semver/patch No public API change. label Dec 11, 2023
@PARAIPAN9
Copy link
Contributor Author

You're very welcome! Thank you for considering the changes, and I completely understand the priority of shipping 1.0.0 first. I'm thrilled to contribute, and eagerly looking forward to the exciting milestone of the first release.

@simonjbeaumont simonjbeaumont added the status/blocked Waiting for another issue. label Dec 12, 2023
@czechboy0 czechboy0 removed the status/blocked Waiting for another issue. label Dec 13, 2023
@czechboy0
Copy link
Contributor

Hi @PARAIPAN9 - the issue might not have been super clear, but the issue was about doing the work of walking the OpenAPI document, and validating all the content type strings that exist there, in the validateDoc function.

That way, we'd catch invalid content types at the start, instead of doing so later when we're trying to generate code.

The errors emitted today, when an invalid content type is encountered, are good enough, we just don't do the validation at the start, so it can lead to a more confusing debugging experience.

Does that make sense? If that task is not something you want to pick up, no worries at all, just let me know and I'd unassign you. 🙂

Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

See comment above, the task is slightly different.

@PARAIPAN9
Copy link
Contributor Author

Ah, thank you for the clarification. It's ok, I'll continue working on it.

@PARAIPAN9 PARAIPAN9 changed the title Add specific errors to content types Validate content type strings in validateDoc Dec 18, 2023
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Thanks - just a few more comments about how we can validate all the content types and emit more information if an invalid content type is found.

Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Great, we're basically there - just added a few more suggestions, but this is the right shape!

Sources/_OpenAPIGeneratorCore/Parser/validateDoc.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Parser/validateDoc.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Parser/validateDoc.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Parser/validateDoc.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Parser/validateDoc.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Great, thank you! Let's just get the CI green and I'll merge it.

@czechboy0
Copy link
Contributor

@swift-server-bot test this please

@PARAIPAN9
Copy link
Contributor Author

Perfect, You’re welcome!!

@czechboy0 czechboy0 merged commit 85fec92 into apple:main Dec 20, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate content-type strings in validateDoc
3 participants