search

widgetti / solara

A Pure Python, React-style Framework for Scaling Your Jupyter and Web Apps
https://solara.dev
MIT License
1.6k stars 104 forks source link

refactor API for workign with external dataframe libs #600

Closed NickCrews closed 2 weeks ago

NickCrews commented 3 weeks ago

working towards https://github.com/widgetti/solara/issues/599

NickCrews commented 3 weeks ago

Man, do you really still have that many 3.6 users?? Let's abandon them soon, they gotta keep up, catering to them isn't worth the fuss!

maartenbreddels commented 3 weeks ago

Not many, but it's a small effort high reward (we had a large client that still needed to support it)

maartenbreddels commented 3 weeks ago

Btw, please do not merge master back into a branch, but rebase instead. (Note to self: write this into our contributing document)

NickCrews commented 3 weeks ago

thanks, sorry. Just blindly hit the default button in the github UI. I wish the default were rebase. Perhaps you can change that default in the repo settings?

maartenbreddels commented 3 weeks ago

Doesn't seem to be an option.

maartenbreddels commented 3 weeks ago

Code wise I 💯 with this, but there are actually many changes in this PR, and although I think you did a good job of separating it into different commits, but I'd like to ask for a few changes. I'd ideally see 4 commits.

The type-fix commit should move to the commits they belong to, and the merge commit should not be there.

Let me know if you are comfortable with this kinda of rebasing, if not, let me know, and we can try to take it over.

Thanks!

NickCrews commented 2 weeks ago

OK, thanks for the review, I agree that is better. FYI I find that rebasing tedious and disincentivizes me from wanting to contribute again, in my own code I wouldn't bother being as scrupulous as this, I would prioritize a little more dev velocity over a really clean git history. You ultimately are the one who has to live with this code so it's your call of course and I respect that, but FYI I would love to avoid rebasing and merge conflicts as much as possible.

maartenbreddels commented 2 weeks ago

Yes, I understand the tradeoff. We've been hit by a few very difficult to debug bugs in the past that were only possibly to solve with fine commits, so we have a tendency to be a bit strict. Also, it's good to clearly think what the intent is of individual commits, but it can be tedious at times (that's why I suggested I could take over).

In any case, thank you 🙏

maartenbreddels commented 2 weeks ago

Btw, I never merge, because I have no idea what it does internally, and I also have no idea how to interpret the git history after a merge back and forward. By having only a linear history in master, and always rebase again that, I have a good mental model of the code.