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

Split declaration? #3193

Closed
HalfWhitt opened this issue Feb 17, 2025 · 6 comments · Fixed by #3195
Closed

Split declaration? #3193

HalfWhitt opened this issue Feb 17, 2025 · 6 comments · Fixed by #3195
Labels
enhancement New features, or improvements to existing features.

Comments

@HalfWhitt
Copy link
Contributor

What is the problem or limitation you are having?

Travertino's declaration.py, at 501 lines, isn't exactly a huge file. But it's got a decent bit going on in it, and will have more once composite_property is added.

Describe the solution you'd like

I'd like to propose splitting the file in two, at roughly the halfway mark: keep Choices and the various properties together, and put BaseStyle in its own file.

If that's a go, then the question would be how precisely to do it. I think the main options are:

  • Split it into two (still top-level) files. It would probably makes sense to rename these to properties and style. I feel like this is probably what we'd do if starting here from scratch, but it means Toga (and maybe user code, on the slim chance anyone's using it) would need to change their imports. We'd also need to keep an import-only declaration module at first, as a compatibility shim for Toga <= 0.4.8.
  • Turn declaration into a subpackage containing the two files, with an __all__ exporting all the needed symbols, so all the imports stay the same.
  • A sort of compromise of the two: two top-level files, named properties and declaration, the latter of which imports all the properties from the former.

My instinct would be the first option, but either of the other two would mean less code churn.

Describe alternatives you've considered

Leave it as-is.

Additional context

No response

@HalfWhitt HalfWhitt added the enhancement New features, or improvements to existing features. label Feb 17, 2025
@freakboy3742
Copy link
Member

Agreed that declaration is reaching the point where breaking it up would make sense. Of the options you've given, I think I concur with your "option 1" as the best approach. With the advent of Toga being able to hard-lock Travertino versions, I'm a lot less concerned about code churn, outside the basic compatibility to survive the Toga 0.5 transition.

The only question I'd have is whether properties might benefit from being a submodule - if we're going to have lots of different types of properties, then it might make sense to split them up, and have properties/validated.py, properties/directional.py, etc.

However, I'm not sure there's much else on the cards other than composite_property, so maybe that's overkill.

@HalfWhitt
Copy link
Contributor Author

The only question I'd have is whether properties might benefit from being a submodule - if we're going to have lots of different types of properties, then it might make sense to split them up, and have properties/validated.py, properties/directional.py, etc.

However, I'm not sure there's much else on the cards other than composite_property, so maybe that's overkill.

Take this with a grain of salt — because I haven't typed a single line of code on it and it's just ideas in my head — but I still suspect that the best way to make the Pack property aliases play nice with __contains__ (#3127) and the dataclass decorator is going to involve pushing the aliasing mechanism down into Travertino. So yeah, you might have a point there.

That would be easy enough to do later though, if and when that materializes.

@HalfWhitt
Copy link
Contributor Author

Speaking of dataclass, composite_property, and possible changes to aliasing — at what point does 0.5.0 happen / what do we want to have done for it? We've got the Rubicon fix and Travertino is added to the repository, which as I recall were the two big ones.

(I also remember saying that the dataclass / code completion for Pack would be a quick thing to toss in for the update, but that was before we added all the property name aliases and deprecations... Hopefully it still won't be too complicated, but it's at least no longer trivial.)

@freakboy3742
Copy link
Member

That would be easy enough to do later though, if and when that materializes.

True - except for the Yet More Import Churn factor...

Speaking of dataclass, composite_property, and possible changes to aliasing — at what point does 0.5.0 happen / what do we want to have done for it? We've got the Rubicon fix and Travertino is added to the repository, which as I recall were the two big ones.

We don't really have a concrete TODO list for a 0.5 release. There's a broad "let's try and get all the backwards incompatibilities done now", but that's about it.

We also want to make sure that 0.4.9 has a chance to get upgraded in the wild... but no matter how long we wait, there's going to be someone with ">= 0.4.5" in their pyproject.toml that is going to be bitten by the upgrade, so that's probably not the best reason to defer a release.

Beyond that, it's really driven by determining when having all the code in main and not in a stable release becomes more of a maintenance headache for us. Right now, there's a lot of good stuff in main, but nothing critical that we get out there because users are getting confused, or bugs are being reported. However, it has been almost 4 months since our last "real" release (discounting 0.4.9 because it's 0.4.8 wearing a mask)... so we're somewhat overdue.

(I also remember saying that the dataclass / code completion for Pack would be a quick thing to toss in for the update, but that was before we added all the property name aliases and deprecations... Hopefully it still won't be too complicated, but it's at least no longer trivial.)

I don't know if @mhsmith has any other thoughts - but if you think it's worth holding off for a week or two while you get the last bits of the dataclass/code completion update in place, I'd be fine with that. If you think it's not worth it (or you start poking around and work out it's going to be a much bigger job), then I'd be happy targeting "end of next week-ish" as a target release date for 0.5.0. That would have the added bonus that the February status update would include a big Toga release as a banner item.

@HalfWhitt
Copy link
Contributor Author

HalfWhitt commented Feb 17, 2025

That would be easy enough to do later though, if and when that materializes.

True - except for the Yet More Import Churn factor...

It shouldn't change any imports, as long as properties has an __init__.py; it would still be from travertino.properties import validated_property either way, right?

I don't know if @mhsmith has any other thoughts - but if you think it's worth holding off for a week or two while you get the last bits of the dataclass/code completion update in place, I'd be fine with that. If you think it's not worth it (or you start poking around and work out it's going to be a much bigger job), then I'd be happy targeting "end of next week-ish" as a target release date for 0.5.0. That would have the added bonus that the February status update would include a big Toga release as a banner item.

I'm not sure how much time I'm going to have for this in the next couple of weeks. If I happen to get to it and it turns out to be pretty simple, cool — but I think I'd plan on it being a post-0.5.0 thing.

@mhsmith
Copy link
Member

mhsmith commented Feb 17, 2025

I don't know if @mhsmith has any other thoughts - but if you think it's worth holding off for a week or two while you get the last bits of the dataclass/code completion update in place, I'd be fine with that. If you think it's not worth it (or you start poking around and work out it's going to be a much bigger job), then I'd be happy targeting "end of next week-ish" as a target release date for 0.5.0.

That sounds fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants