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

allow specify multiple schema #190

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexander-smyslov
Copy link

Allow specify multiple schemas in command line like :

migra --schema schema1 --schema schema2

@djrobstep
Copy link
Owner

Hi there, thanks so much for submitting this PR, and sorry it's taken so long to get to reviewing. I stepped away from active maintainer duties for a while but have now returned.

With the PR as it is, I worry about it adding complexity to the external command line flow. I think a cleaner approach might be to handle multiple schemas within the migration object, rather than changing the command line behaviour. So the general flow would be: m = Migration(a, b, schemas=bla) and then it would be handled within the migration object.

What do you think?

@alexander-smyslov
Copy link
Author

Hi there, thanks so much for submitting this PR, and sorry it's taken so long to get to reviewing. I stepped away from active maintainer duties for a while but have now returned.

With the PR as it is, I worry about it adding complexity to the external command line flow. I think a cleaner approach might be to handle multiple schemas within the migration object, rather than changing the command line behaviour. So the general flow would be: m = Migration(a, b, schemas=bla) and then it would be handled within the migration object.

What do you think?

I will try to adopt a solution with your suggestion.

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