Skip to content

Split htpy into multiple private modules #109

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

Closed
wants to merge 6 commits into from

Conversation

jodal
Copy link
Contributor

@jodal jodal commented Apr 17, 2025

When looking at the source code I had trouble deciding where to add @with_children, so I started wondering if the source code could be split up and organized in a way that made maintenance and extension easier.

This is the kind of thing that probably should be discussed first, but where it is also easier to discuss it when one can see how it could end up, so I simply went ahead and made an attempt at splitting up htpy in a way I found sensible. I trust you to say so and close the PR if you do not like this direction at all!


The first commit should be considered alone. It does the minimum required to get to working state, keeping all names unchanged.

The following commits are more opinionated, as they remove _ prefixes from private members of private modules that are used across multiple modules, so that we can avoid a lot of pyright ignores. I think the end result here is a bit nicer, but I can also see the benefit of always having a _ prefix on everything that is private.

@jodal jodal force-pushed the push-ustlntnzrptl branch from 0cabfb9 to 3b4a55e Compare April 17, 2025 13:37
jodal added 2 commits April 17, 2025 15:38
We only import markupsafe in private modules, so it doesn't need to be
aliased to avoid leaking the import.
@jodal jodal force-pushed the push-ustlntnzrptl branch from 3b4a55e to 03010d8 Compare April 17, 2025 13:38
jodal added 4 commits April 17, 2025 16:00
It is still private as it is defined and exclusively used in
private modules or inside functions.
These are still private as they are defined and exclusively used in
private modules.
These are still private as they are defined and exclusively used in
private modules.
These are still private as they are defined and exclusively used in
private modules.
@jodal jodal force-pushed the push-ustlntnzrptl branch from 03010d8 to c251a10 Compare April 17, 2025 14:00
@pelme
Copy link
Owner

pelme commented Apr 17, 2025

Hi @jodal

htpy used to be split in multiple files: https://github.com/pelme/htpy/tree/b46f919777afb4117b2058f135c689adf39b23b4/htpy. I then thought it was more annoying than useful and at some point moved it all to one file. It has grown a bit since then but I like having all of the htpy "core" in a single file since it makes it easy to find everything. It is still <500 lines (if you do not count the list of native html elements).

I think it is good that utils like htpy.django and htpy.html2htpy lives in other modules. The @with_children decorator could live in its own file too I think.

Maybe some widely used helpers such as @with_children and something else can be part of htpy. I really think/hope that core htpy is kind of "done". With the Renderable protocol and soon async streaming (#38 ), I do not foresee many changes to the core of htpy. But we will see I guess! 🙂

What I find lacking is the docs. The docs already outweigh htpy core by 3:1 in line count and I think that they could be expanded with better tutorials, guides and examples and explanations.

The tests are split up into multiple files, they are also some ~1500 lines.

Sorry for the rant and thanks for the idea+suggestion. I would rather not merge this and keep it as-is for now. If for some reason htpy grows to thousands of lines or so, I would be happy to discuss this again!

@jodal
Copy link
Contributor Author

jodal commented Apr 18, 2025

I think that htpy being a single file was a contributing factor to me starting to read the code the first time. By now, I've read the source code many times. However, I still get lost in it sometimes and find myself scrolling or searching to pick up the thread.

I have to argue that since last time the package was split in multiple files the code has grown at least contexts and fragments. Of things around the corner, the async iteration adds quite a bit of code and @with_children would also add a bit (however, it could be in its own file irrespective of where you import and use it from). If HTML element attribute code completion (#110) were to be implemented, that would surely add quite a lot of code to declare all the allowed attributes of each HTML element.

Even if fighting hard to keep htpy conceptually simple, which I fully agree with, the full implementation will at some point in the near future no longer be something you'll read from end to end in a single sitting. If it was split up, you will hopefully still find yourself reading through the pieces that are on the abstraction level you care about, without digging into e.g. the details of how attributes are parsed on your first pass through.

Final argument: If you haven't already done so, I think it could be worth checking out the branch and looking around in an editor instead of just in the diff view here. That gives a way better impression of the end result, I think.

@jodal jodal changed the title Spit htpy into multiple private modules Split htpy into multiple private modules Apr 23, 2025
@jodal jodal closed this May 3, 2025
@jodal jodal deleted the push-ustlntnzrptl branch May 3, 2025 08:39
@pelme
Copy link
Owner

pelme commented May 25, 2025

hi @jodal, again - thanks for bringing this up and sorry for being slow to reply.

I have been thinking more about this and changed my mind on this PR. I started playing with implementing the rendering/base types in rust and came to the realisation that the current structure is making that hard. Not sure if it is worthwhile to implement htpy in rust but I am curious to give it a try and see how it affects performance and what a rust implementation would look like.

Anyways, if you are still interested in pursuing this PR, I would be happy to merge this PR if it was updated with the current main. Would then make sure to merge it quickly then to avoid merge problems. If you are not interested to keep working in this, that is totally understandable, let me know and I will finish it!

@jodal jodal restored the push-ustlntnzrptl branch May 25, 2025 21:27
@jodal
Copy link
Contributor Author

jodal commented May 25, 2025

Happy to hear that!

I've opened a new PR #119 with an updated version of this PR.

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.

2 participants