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

[IMP] Ask user input if the target branch already exists #60

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

Conversation

ivantodorovich
Copy link
Collaborator

No description provided.

Copy link
Member

@joao-p-marques joao-p-marques left a comment

Choose a reason for hiding this comment

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

Code review

Just a minor comment. Otherwise, looks great 👍 Thanks for the addition

Could you also add a test to this?

@@ -98,53 +99,70 @@ def __init__(
# get migration scripts, depending to the migration list
self._get_migration_scripts()

def _execute_shell(self, shell_command, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

I don’t really see the advantage in this patch to the method… IMHO, it ends up complicating a bit the different methods and the overall structure… I would leave it as is, passing the pass keyword argument each time…

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ivantodorovich what is your point of view, regarding that comment ?

thanks !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's to avoid having to pass the directory_path every time (and to avoid issues in case we forget about it)
For example, see the issue we had here: #64

As when running migrations, we mostly want to execute the shell in the directory of the repository (provided by the --directory argument)

@legalsylvain
Copy link
Collaborator

legalsylvain commented May 15, 2023

Hi.

I recently added pre-commit file, so a lot of files changed. A simple rebase should be done.
sorry for the extra work.

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