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

Fix RelationKey unquoted issue. Fixes #137 #228

Merged
merged 7 commits into from Aug 17, 2021

Conversation

Brooke-white
Copy link
Contributor

#184 reopened

  • Addresses PR feedback from @novotl regarding RelationKey.unquoted() return type & test modifications

  • Addresses PR feedback from @graingert regarding use of pytest.mark.parameterize.

  • MIT compatible

  • Tests

  • Documentation

  • Updated CHANGES.rst

tox has been run, no test failures seen.

Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

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

Integration tests are passing, so the logic and tests here look good.

I left one style suggestion, but I'm open to pushback on that.

I don't think any documentation change is needed for this, but we do need to update CHANGES.rst. Can you go ahead and add a change entry under "0.8.5 (unreleased)" that links to this PR?

Comment on lines 219 to 225
def unquote(part):
if (
part is not None and part.startswith('"') and
part.endswith('"')
):
return part[1:-1]
return part
Copy link
Member

Choose a reason for hiding this comment

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

This would feel more pythonic to me if we moved this to be a class method _unquote rather than defining this function nested inside the unquoted method. I'm not used to seeing nested functions.

@Brooke-white
Copy link
Contributor Author

thanks for the review! I've pushed changes addressing your feedback.

Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

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

Looks ready to go. I'm going to commit the staticmethod change and rerun integration tests, then I'll go ahead and merge once it's green.

Thanks for this first contribution, @Brooke-white!

sqlalchemy_redshift/dialect.py Outdated Show resolved Hide resolved
@jklukas jklukas merged commit b6ccbc3 into sqlalchemy-redshift:main Aug 17, 2021
@jklukas
Copy link
Member

jklukas commented Aug 17, 2021

Integration tests weren't able to complete due to credit balance with Travis-ci.com; I'm contacting their support, but in the meanwhile, going ahead and merging this change.

@Honza-m Honza-m mentioned this pull request Jan 20, 2023
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.

None yet

3 participants