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

add option to use dataclasses instead of attrs in generated code #1158

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eli-bl
Copy link
Collaborator

@eli-bl eli-bl commented Nov 5, 2024

The last time this idea came up (#743), @dbanty felt that there wasn't a compelling reason to switch from attrs. I decided anyway to see how much of a lift it would be— in the generated code, that is, not in the generator. A few reasons:

  1. In my main use case, which is creating a library to be used by all sorts of third-party applications, there's an incentive to minimize external dependencies if possible.
  2. Even though attrs is very popular, I think it's likely to become somewhat less of a standard as Python's dataclasses module keeps on copying features from it.
  3. It's true that attrs has some features that dataclasses doesn't, but we're not really using those features anyway in the generated code (with one exception that I'll mention below). Mostly we're just using the simplest possible attribute declaration pattern in model classes.

As I expected, implementing this for the model classes was pretty simple. The only other place we were using attrs in the generated code was in the Client and AuthenticatedClient classes, and there things got trickier.

In those classes, we were taking advantage of attrs's alias option in order to define underscored attributes that the app isn't meant to access directly, but that can still be set by name without the underscore in the initializer. You can't do that with dataclasses. However, since the app isn't meant to access them directly, I felt freer to change things there... and attrs/dataclass-type behavior is not really inherent to what the client classes are for, from the app's point of view, it's just an implementation detail.

So, in use_dataclasses mode, I changed the clients to plain Python classes with no attrs/dataclass decorators—and moved those Httpx-related attributes into a separate dataclass, so we could still take advantage of copy-and-set behavior for builder methods. That is a fairly different implementation and code structure, which makes the Jinja template a bit hairy, but you can see what it ends up looking like here. However, when you're not in use_dataclasses mode, the implementation does not change at all (you can see there are no changes to existing code in golden_record), so I believe this is not a breaking change.

(We could decide to unify those implementations by backporting those changes to the not-using-dataclasses mode, so the only differences between the two would be the decorator names. I do think that'd be a little bit cleaner— the Httpx options do all go together, and I don't think there's a compelling reason to have them be separate attributes on the client class. And, again, apps were never meant to access those directly... but in Python, we couldn't prevent that, so that could be considered a breaking change depending on your standards for what is a public/supported interface.)

@eli-bl
Copy link
Collaborator Author

eli-bl commented Nov 5, 2024

Also, if my #1156 is accepted, this is another kind of thing that would be well suited to unit tests of the generated code. For instance, test classes for model behavior could be parameterized to run everything in both attrs mode and dataclasses mode.

@eli-bl eli-bl marked this pull request as ready for review November 5, 2024 17:53
@eli-bl eli-bl requested a review from dbanty November 5, 2024 17:53
@eli-bl eli-bl marked this pull request as draft November 14, 2024 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant