Skip to content

refactor API for workign with external dataframe libs #600

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

Merged
merged 4 commits into from
Apr 19, 2024

Conversation

NickCrews
Copy link
Contributor

working towards #599

Copy link
Contributor

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some minor issues with older Python versions

@NickCrews
Copy link
Contributor Author

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
Copy link
Contributor

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

@maartenbreddels
Copy link
Contributor

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

@NickCrews
Copy link
Contributor Author

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
Copy link
Contributor

Doesn't seem to be an option.

@maartenbreddels
Copy link
Contributor

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.

  • refactor: move df_records() from DataTable to dataframe utils
  • refactor: move slicing of dataframe from DataTable to dataframe utils
  • fix: use .iloc for pandas dataframe slicing
  • refactor: move df_coloumn into dataframe utils

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!

before, we used df[i1, i2] for slicing, but we should be using
.iloc[i1:i2] instead since we are using positional indexing.
This could either be a bugfix or a breaking change
depending on your opinion.
@NickCrews
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

@maartenbreddels maartenbreddels merged commit bee614a into widgetti:master Apr 19, 2024
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