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

Table identifier #97

Merged
merged 3 commits into from
Nov 2, 2016
Merged

Table identifier #97

merged 3 commits into from
Nov 2, 2016

Conversation

jklukas
Copy link
Member

@jklukas jklukas commented Sep 30, 2016

Todos

  • MIT compatible
  • Tests
  • Documentation
  • Updated CHANGES.rst

This builds on #78 and avoids the trouble of parsing qualified table names by instead introducing a RelationKey namedtuple that tracks those values separately.

Does this look like a good approach?

@graingert
Copy link
Member

there's something up with your rebase

@jklukas
Copy link
Member Author

jklukas commented Oct 5, 2016

I don't know what was up with the rebase. I did some surgery and I think this is correct now.

@jklukas
Copy link
Member Author

jklukas commented Oct 5, 2016

Looks like there's an error in the tests here. Will investigate.

@jklukas
Copy link
Member Author

jklukas commented Oct 5, 2016

Merged in a change that takes out the now-unnecessary test about splitting schema and table name.

@graingert
Copy link
Member

@jklukas your rebase is still odd, I pushed f1cc229 to unbreak stuff

@graingert
Copy link
Member

I recommened removing it and re-applying your rebase

@jklukas jklukas force-pushed the table-identifier branch 2 times, most recently from f16dfba to 5929d27 Compare October 5, 2016 18:46
@jklukas
Copy link
Member Author

jklukas commented Oct 5, 2016

Pulled in that change and rebased again. Tests are passing this time, so ready for review.

@jklukas
Copy link
Member Author

jklukas commented Oct 5, 2016

@graingert - This is a bug fix, so I don't see the need for any documentation changes. Does this warrant something in the changelog?

@graingert
Copy link
Member

Definitely change the changelog

@jklukas
Copy link
Member Author

jklukas commented Oct 5, 2016

Added in a line in CHANGES.rst and squashed.

@graingert
Copy link
Member

@jklukas is this ready to go?

@graingert graingert added this to the 0.6.0 milestone Nov 2, 2016
@jklukas
Copy link
Member Author

jklukas commented Nov 2, 2016

Yes, this should be good to merge. Are you +1, @graingert ?

@graingert graingert merged commit 131dda9 into master Nov 2, 2016
@graingert graingert deleted the table-identifier branch November 2, 2016 17:32
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.

3 participants