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

do not modify the click command name when it is a MultiCommand #42 #44

Merged
merged 2 commits into from
Feb 14, 2023

Conversation

leo-schick
Copy link
Member

See #42

Supports implemeting nicer cli syntax e.g.

flask db migrate
flask pipeline run

# or with custom naming `mara`
mara db migrate
mara pipeline run

which is more intuitive than:

flask mara_db.migrate
flask mara_pipelines.ui.run

@leo-schick leo-schick requested a review from jankatins February 7, 2023 16:03
@jankatins
Copy link
Member

Is this backwards compatible? The nice thing about having prefixes was that getting conflicts is a lot less likely, given that you have user contributed command and commands from libraries.

@leo-schick
Copy link
Member Author

As long as you just add the new click.Group and don't throw out the current commands which are based on click.Command, it is backward compatible.
I suggest to add a click.Group to all of the mara packages and throw out the old commands with the next major release of the package (e.g. for mara-db that would be version 5.0). This makes a migration smooth for those using the old commands in scripts.

I suggest to avoid conflicts by just using different command namespaces.

For example: Imagine I build my custom function which creates a database in PostgreSQL when the database does not yet exit. Exposing this would be typically in the mara-db package. The shell command should be flask db create <db_alias>. But since it is custom code, you would use your own namespace flask <my_company> db create <db_alias>.

An alternative approach would be merging the click groups by using click.CommandCollection. I have not looked into this. I currently suggest to use clear namespaces. I see a potential risk when someone experiments with external modules outside the mara suite which might use the same namespace. I guess this can be tackled when there is an case for this.

I strongly go for clear namespaces here. flask pipeline run or even mara pipeline run is 100% more intuitive as flask mara_pipelines.ui.run

@leo-schick
Copy link
Member Author

@jankatins What do you think about putting this logic into a separate function so that it is patchable? Then people which want to use the old option still have the chance to do so.

@leo-schick
Copy link
Member Author

@jankatins patchable function added

Copy link
Member

@jankatins jankatins left a comment

Choose a reason for hiding this comment

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

LGTM

@leo-schick leo-schick merged commit 1d1577e into main Feb 14, 2023
@leo-schick leo-schick deleted the issue-42 branch February 14, 2023 10:13
@leo-schick leo-schick added this to the 2.4.0 milestone Feb 21, 2023
leo-schick added a commit that referenced this pull request Feb 23, 2023
* do not modify the click command name when it is a MultiCommand #42

* add patchable function for register_command
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