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

Idiomatic naming strategy as opt-in #679

Merged
merged 39 commits into from
Dec 18, 2024

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented Nov 22, 2024

Motivation

Implementation of #683.

Modifications

  • Implemented the SOAR-0013 idiomatic naming strategy
  • Added name overrides
  • Switched most reference tests to use the new strategy
  • Updated some docs (I'll update the rest after this lands and gets released, then I can update Examples and IntegrationTest)

Result

SOAR-0013 implemented.

Test Plan

Updated and added new unit tests.

@czechboy0 czechboy0 changed the title [WIP] Optimistic naming strategy as opt-in [WIP] Idiomatic naming strategy as opt-in Dec 10, 2024
@czechboy0 czechboy0 added the 🆕 semver/minor Adds new public API. label Dec 11, 2024
@czechboy0 czechboy0 changed the title [WIP] Idiomatic naming strategy as opt-in Idiomatic naming strategy as opt-in Dec 16, 2024
@czechboy0 czechboy0 marked this pull request as ready for review December 16, 2024 07:35
Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

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

Thanks @czechboy0! Overall this looks good. There are a couple of things I think we should change: config management, use in tests, etc.

Sources/_OpenAPIGeneratorCore/Config.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Config.swift Outdated Show resolved Hide resolved
Sources/_OpenAPIGeneratorCore/Config.swift Outdated Show resolved Hide resolved
Sources/swift-openapi-generator/FilterCommand.swift Outdated Show resolved Hide resolved
Sources/swift-openapi-generator/GenerateOptions.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

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

Thanks for getting this one this far @czechboy0 :)

As per this comment and our additional discussion offline, I think we need to revisit the design of the TypeAssigner and the coupling with the naming logic.

Previously we just called into some closure which was in the context, which took no options. Now we have two strategies we've added a lot of complexity to the assigner in how it should call the closure, and since we're dealing with casing, we're also having to call the idiomatic strategy in a different way for different contexts in which it's used.

We didn't love the design before, but trying to stick with it for this feature is resulting in a lot of increased complexity and I think we should refactor to decouple the assigner and namer and create a better abstraction.

I'd personally prefer to see that as part of this PR, rather than moving things in the wrong direction temporarily.

But since this PR does add the feature we need, with the design we have, we can follow it up after.

Copy link
Collaborator

@simonjbeaumont simonjbeaumont 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 really nice—thanks!

@simonjbeaumont simonjbeaumont enabled auto-merge (squash) December 18, 2024 15:49
@simonjbeaumont simonjbeaumont merged commit ad7f0e7 into apple:main Dec 18, 2024
32 checks passed
@czechboy0 czechboy0 deleted the hd-naming-strategy-optimistic branch December 18, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants