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.

Sign up for GitHub

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

Jump to bottom

Allow multiple models in sqlalchemy #30

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Allow multiple models in sqlalchemy #30

wants to merge 10 commits into from

Conversation

AndiZeta
Copy link

See issue #24

azelcer and others added 10 commits September 5, 2022 18:34
OliverHofkens
OliverHofkens reviewed Oct 28, 2022
View reviewed changes
Copy link
Member

@OliverHofkens OliverHofkens left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this, looks great!
I added one minor linting nitpick, but I might just quickly fix this myself if I have some time today so this can be released soon.
I might also add some test cases on this behavior.

odata_query/sqlalchemy/shorthand.py
@@ -1,31 +1,66 @@
from typing import List
Copy link
Member

Choose a reason for hiding this comment

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

The imports aren't organized correctly. We use isort to automate this process, as described in https://github.com/gorilla-co/odata-query/blob/master/CONTRIBUTING.rst#local-development. I did notice that our Github Workflow wasn't configured to run on Pull Requests, otherwise this would've been more clear, sorry! This is improved for future PRs.

Copy link
Author

Choose a reason for hiding this comment

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

My bad. I did run blake but forgot to run isort. I had some issues with poetry, so I wasn't able to properly test the PR. Please do not hesitate to make any changes you think are necessary, or let me know if it is something I can do.

@OliverHofkens OliverHofkens linked an issue Oct 28, 2022 that may be closed by this pull request
Managing more than one entity in AstToSqlAlchemyClauseVisitor #24
Open
@OliverHofkens OliverHofkens added the enhancement New feature or request label Oct 28, 2022
@OliverHofkens
Copy link
Member

Hey @AndiZeta, any chance you could squash your commits into 1?
I'm planning on merging this soon, but rebasing the 10 commits to get to the current state of master is kind of a pain 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OliverHofkens OliverHofkens OliverHofkens left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Managing more than one entity in AstToSqlAlchemyClauseVisitor
3 participants
@AndiZeta @OliverHofkens @azelcer