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

CLN: sql static method #36410

Closed
wants to merge 42 commits into from
Closed

CLN: sql static method #36410

wants to merge 42 commits into from

Conversation

erfannariman
Copy link
Member

  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

@erfannariman
Copy link
Member Author

Not sure why my commits from #35224 are included in this PR. Maybe because I changed my email in config user.email. But the changes I made for this PR are in sql.py

@erfannariman erfannariman changed the title Cln sql static method CLN: sql static method Sep 17, 2020
@MarcoGorelli
Copy link
Member

Not sure why my commits from #35224 are included in this PR. Maybe because I changed my email in config user.email. But the changes I made for this PR are in sql.py

I'd guess it's because you created a new branch from your previous one and then added new commits on top of it, instead of resetting to upstream/master before starting new work.

You can get rid of unwanted commits with git rebase -i upstream/master and then dropping any commits you don't want (note that you'll then need to use the -f flag when pushing)

@erfannariman
Copy link
Member Author

Not sure why my commits from #35224 are included in this PR. Maybe because I changed my email in config user.email. But the changes I made for this PR are in sql.py

I'd guess it's because you created a new branch from your previous one and then added new commits on top of it, instead of resetting to upstream/master before starting new work.

You can get rid of unwanted commits with git rebase -i upstream/master and then dropping any commits you don't want (note that you'll then need to use the -f flag when pushing)

Thanks, I can do that, but the changes now look correct, and it does not conflict with the base branch, it's fine like this as well I guess?

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Sep 17, 2020

I think it should be fine, yes :) While you're here modifying these files, can you also add type annotations to the signatures you've changed (where possible)?

@@ -1079,7 +1079,8 @@ def _sqlalchemy_type(self, col):

return Text

def _get_dtype(self, sqltype):
@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

-1 on static methods generally; you can move to a free function if you really want

Copy link
Member

Choose a reason for hiding this comment

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

I agree we should generally be using staticmethods only sparingly, but IMO here that is fine. This method is only used once, just above, and keeping it here keeps it closer to where it is used.
The same for _fetchall_as_list. And the get_table is actually overriding a parent class method, so shouldn't be moved to a free function.

So I prefer to keep those functions where they are. Whether they are normal methods or staticmethods, I personally don't care. So if others don't want to use staticmethods, I would just leave this as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess with the -1 and with the leave as is, I will close this PR.

@jreback jreback added the IO SQL to_sql, read_sql, read_sql_query label Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO SQL to_sql, read_sql, read_sql_query
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants