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

Access properties with explicit self in encode(to encoder:) #699

Merged

Conversation

simonjbeaumont
Copy link
Collaborator

@simonjbeaumont simonjbeaumont commented Dec 11, 2024

Motivation

In #696 we avoided clashing with a property called container in certain schemas by making use of explicit self in init(from decoder:). We didn't have access to the OpenAPI document and hadn't caught the case where this was used in a schema that supported additional properties, where we also need to avoid the clash in encode(to encoder:).

Modifications

  • Access properties with explicit self in encode(to encoder:).

Result

Test Plan

More snippet tests and reference tests.

Copy link

@Dragna Dragna left a comment

Choose a reason for hiding this comment

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

it's working fine on my project, everything compiles 🙂.

Thank you again for your help 🙏 .

I'll close the PR I created

@simonjbeaumont
Copy link
Collaborator Author

@Dragna Thanks for confirming that this resolves the issues with your particular OpenAPI document.

Again, apologies for the PR race—I didn't mean for this to supersede your efforts.

If you ever want to contribute to the project again, we'd be gladly accept pull requests.

@simonjbeaumont simonjbeaumont marked this pull request as ready for review December 11, 2024 10:05
@simonjbeaumont simonjbeaumont merged commit 2e1b6fc into apple:main Dec 11, 2024
32 of 33 checks passed
@Dragna
Copy link

Dragna commented Dec 11, 2024

@Dragna Thanks for confirming that this resolves the issues with your particular OpenAPI document.

Again, apologies for the PR race—I didn't mean for this to supersede your efforts.

If you ever want to contribute to the project again, we'd be gladly accept pull requests.

Thank you for your help, and don't mind the fact that I created a PR, I was lost in the snippet test file and couldn't find the right place where I should have updated the test.

Thanks again for taking time on this and helping us 🙏.

simonjbeaumont added a commit that referenced this pull request Dec 11, 2024
…and encoders (#700)

### Motivation

In order to address #695 we made some targeted fixes (#696, #699) to use
explicit-self to avoid clashing with schema property names. This
addressed a clash with a specific variable with properties of only
certain schemas, but there are more places where we might encounter
future issues. Specifically, we have other variable names which might
cause a clash (e.g. `storage` and `discriminator`), and there are other
flavours of initializers, encoders, and decoders that were not updated
in the targeted fix.

In this PR, we update all access and assignment in the generated code
dealing with schemas to use explicit-self.


### Modifications

- Use self for member access and assignment in all initializers,
decoders, and encoders.
- Update the reference tests and snippet tests.

### Result

Removed some cases where valid OpenAPI docs would produce non-compiling
code.

### Test Plan

Snippet and reference tests. The latter is actually compiled during the
CI.
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.

Can't use container as a property name of a model
3 participants