pglet / pglet-python

Python client for Pglet - build real-time interactive web apps in Python
MIT License
18 stars 7 forks source link

Support dicts as Grid items and some finessing #60

Closed mikaelho closed 2 years ago

mikaelho commented 2 years ago

This PR is intended more as a vehicle for discussion than something you might merge as-is (although does work as-is).

Only functionality changes are in these commits:

Then there is a copy-paste update to tests to make them run with the new Commands.

For discussion:

FeodorFitsner commented 2 years ago

Ha, black is on HN home page right now: https://news.ycombinator.com/item?id=30130315 :)

mikaelho commented 2 years ago

Ha, black is on HN home page right now: https://news.ycombinator.com/item?id=30130315 :)

👍🏻 Opinions differ, but I must say that the fundamentally non-value-adding tab discussion that immediately follows the first post there is, to me, a testament to the value of black.

But, on a more functional note, I realized the PR does not yet take care of the updates to the item values, need to add that.

mikaelho commented 2 years ago

Using and updating dicts as Grid item list values works now with the Grid sample.

FeodorFitsner commented 2 years ago

PR looks great, thank you! Working with dictionaries in Grid is definitely a must-have addition. Grid test is be a good example for other tests. Also, I'm going to reformat everything using Black in a separate branch and add it as a requirement to contributing guide.

I'm thinking... should we split Grid classes into separate files under grid directory? Having them all in grid.py is a kind of inconvenient. What does idiomatic Python say?

mikaelho commented 2 years ago

To my knowledge there is no idiomatic Python rule dictating how many classes there should be per file. Thus it is a matter of preference (and maybe capabilities of your editor) when a single file would be considered unwieldy and in need of being split up. grid.py is still on the smaller side and quite far from my personal threshold.

But the single file can be made much more approachable by placing the top class (Grid) first in the file, and go down in granularity as you read down the file. That is somewhat "Pythonic", as Python has no need for forward declarations in most cases.

To demonstrate, I am pushing a commit where I have just quickly rearranged the order of the classes.

mikaelho commented 2 years ago

Oh, and I did delve into beartype a bit more, it does support Python >= 3.7, so no concerns in that regard.

FeodorFitsner commented 2 years ago

Sounds great, let's merge this PR. I'm going to push https://github.com/pglet/pglet-python/pull/61 with Black formatting and annotations later this week.