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.

Sign up for GitHub

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

Jump to bottom

Emit JSON path reference in comments for all generated types #196

Merged
merged 10 commits into from
Aug 17, 2023
Merged

Emit JSON path reference in comments for all generated types #196

merged 10 commits into from
Aug 17, 2023

Conversation

takeshi-1000
Copy link
Contributor

@takeshi-1000 takeshi-1000 commented Aug 15, 2023 •
edited
Loading

Motivation

Resolve #13

Modifications

Applied commentable to the declaration variable in Traslator so that a comment in Types.swift is added indicating that it was generated from the Json path of the openapi document

Result

Some comments in Types.swift is generated, like Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources/Petstore/Types.swift.

Test Plan

Modify Type.swift in the reference source

czechboy0
czechboy0 requested changes Aug 15, 2023
View reviewed changes
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.

This is definitely the right direction! Added a few comments, we can get this PR cleaned up and landed, and then you can continue to address any remaining missing comments in a separate PR.

@takeshi-1000
Copy link
Contributor Author

Thanks for the quick confirmation! The fix is ​​still incomplete, but I just put in something that can be applied immediately.

@czechboy0
Copy link
Contributor

@swift-server-bot add to allowlist

@czechboy0
Copy link
Contributor

Thanks for the quick confirmation! The fix is ​​still incomplete, but I just put in something that can be applied immediately.

Sure – can you please just resolve the CI failures and we could land this PR? Then you can continue to work on the missing bits in a separate PR.

@takeshi-1000
modify some code
4188460 
- fix around `responseKind`
- make `isComponent` more reliable
- add comment for some methods and property.
- addjust soundness
@takeshi-1000
modify Types.swift
709e885
@takeshi-1000 takeshi-1000 marked this pull request as ready for review August 16, 2023 09:45
@takeshi-1000
Copy link
Contributor Author

@czechboy0
I modify some code, could you check please?
Also As you suggested, I plan to address some other issues in another PR.
(There are places where userDescription is not reflected in comments, and characters are implemented with hard code)
Is it okay to open these issues?

czechboy0
czechboy0 requested changes Aug 16, 2023
View reviewed changes
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.

A few more stylistic nits to ensure better readability.

Sources/_OpenAPIGeneratorCore/Translator/RequestBody/translateRequestBody.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Translator/RequestBody/translateRequestBody.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Translator/Responses/translateResponse.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Translator/Responses/translateResponse.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Translator/Responses/translateResponse.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Translator/Responses/translateResponseHeader.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Translator/TypeAssignment/TypeName.swift Outdated Show resolved Hide resolved
@takeshi-1000 takeshi-1000 mentioned this pull request Aug 17, 2023
Closed
@takeshi-1000
Copy link
Contributor Author

takeshi-1000 commented Aug 17, 2023 •
edited
Loading

I open this issue #206 . (I don't mind if you change the charge to me)
Also, the hardcoding issue is closely related to this pull request, so I'm going to publish it after this pr merged.

@czechboy0
Copy link
Contributor

@takeshi-1000 I pushed a few more changes to your branch and will now merge once CI lands. Thanks for taking this on!

czechboy0
czechboy0 approved these changes Aug 17, 2023
View reviewed changes
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.

Updated with one more commit, lgtm.

@czechboy0 czechboy0 enabled auto-merge (squash) August 17, 2023 08:08
@czechboy0 czechboy0 merged commit b59b8a3 into apple:main Aug 17, 2023
@takeshi-1000 takeshi-1000 deleted the add-comment-for-generated-types branch August 17, 2023 10:26
@takeshi-1000
Copy link
Contributor Author

Thank you for suporting!

@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czechboy0 czechboy0 czechboy0 approved these changes

Assignees
No one assigned
Labels
🔨 semver/patch No public API change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Emit JSON path reference in comments for all generated types
2 participants
@takeshi-1000 @czechboy0