Refactor `CommandLine` command registration #1611

mikeroll · 2025-02-23T10:22:18Z

Refactors CommandLine internals so that it's easier to plug custom commands into the cli, in addition to the builtins alembic.command.
No public interface changes whatsoever.

The topic was somewhat discussed here #1456

Description

The point from the linked discussion still applies - this does not enable any kind of plugin system, one would still need to have their own subclass of CommandLine and call it from their own script. However, the implementation of such a subclass should be much cleaner now. I would expect it to look like so:

def frobnicate(config: Config, revision: str) -> None:
    """Frobnicates according to the frobnication specification.

    :param config: a :class:`.Config` instance
    :param revision: the revision to frobnicate
    """

    config.print_stdout(f"Revision {revision} successfully frobnicated.")


class MyCommandLine(CommandLine):
    def _generate_args(self, prog: str | None) -> None:
        super()._generate_args(prog)

        self._register_command(frobnicate)


if __name__ == "__main__":
    MyCommandLine().main()

❯ python cli.py frobnicate 42
Revision 42 successfully frobnicated.

Checklist

This pull request is:

A short code fix
- please include the issue number, and create an issue if none exists, which
  must include a complete example of the issue. one line code fixes without an
  issue and demonstration will not be accepted.
- Please include: Fixes: #<issue number> in the commit message
- please include tests. one line code fixes without tests will not be accepted.

This is purely internal refactoring, so no new tests are required. For extra confidence I:

ran tests with tox locally;
compared the text of alembic --help before and after this change and made sure there was no diff.

Have a nice day!

Fixes: sqlalchemy#1610

mikeroll · 2025-02-27T14:16:50Z

My bad with the type hints, should be green now.

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 369059e of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-03-04T17:39:36Z

New Gerrit review created for change 369059e: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

zzzeek

OK well, the movement of methods and all is fine, but the goal of the PR here means that it's not that simple.

the goal is "make it easier for third parties to subclass CommandLine". This does some initial structure for that, but it doesnt do all these other things:

document any of these methods. how would I know what _register_command does?
make it clear which methods are safe - why are these methods underscored if they are now "public" ?
provide an example in the docs how to subclass CommandLine
provide any unit tests to ensure that a user-custom CommandLine implementation continues to work, otherrwise there's nothing to prevent someone changing CommandLine again here in some arbitrary way
whether or not this is backwards compatible with someone who already did their own CommandLine._generate_args should be established also, it looks like that's maintained here

overall I like making CommandLine subclassable but that's public API, and it has to be done as a public API which means: 1. docs 2. proper conventions 3. API stability 4. tests

sqla-tester

Federico Caselli (CaselIT) wrote:

code review left on gerrit

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

sqla-tester · 2025-03-04T19:55:42Z

alembic/config.py

+        self.parser = parser
+
+    def _register_command(self, fn: Any) -> None:
+        fn, positional, kwarg, help_text = self._parse_command_from_function(fn)


Federico Caselli (CaselIT) wrote:

we could avoid returning the fn here, since it's what we provide as argument

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

sqla-tester · 2025-03-04T19:55:42Z

alembic/config.py

+            if (
+                arg == "revisions"
+                or fn in self._POSITIONAL_TRANSLATIONS
+                and self._POSITIONAL_TRANSLATIONS[fn][arg] == "revisions"


Federico Caselli (CaselIT) wrote:

we have already handle _POSITIONAL_TRANSLATIONS so I don't think we need this or part

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

True, I've removed this.

sqla-tester · 2025-03-04T19:55:42Z

alembic/config.py

-                    fn.__name__, help=" ".join(help_text)
+                subparser.add_argument(
+                    "revisions",
+                    nargs="+",


Federico Caselli (CaselIT) wrote:

let's parametrize this using a dict.

we could rename _POSITIONAL_HELP and make it a dict of dicts that with the kwargs for the add_argument

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

Good idea, we now have _POSITIONAL_OPTS for this.

sqla-tester · 2025-03-04T19:55:42Z

alembic/util/compat.py

@@ -55,9 +56,9 @@ def close(self) -> None:
 def importlib_metadata_get(group: str) -> Sequence[EntryPoint]:
    ep = importlib_metadata.entry_points()
    if hasattr(ep, "select"):
-        return ep.select(group=group)
+        return cast(Sequence[EntryPoint], ep.select(group=group))


Federico Caselli (CaselIT) wrote:

strange that this is now needed

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

Yeah I though I had to fix this now, but turns out I was running different python + mypy versions than CI. Reverted this.

mikeroll · 2025-03-06T12:09:25Z

@zzzeek Thanks for your thoughtful comments!

In this PR I actively avoided doing all the "public interface" work that you mentioned in points 1-4. To be honest, I was intially going to stop at that, but I fully agree that the new interface eventually has to be public to be of any real value.

Regarding point 5 though: yes, the changes to _generate_args are backwards compatible, but in the same vein of public vs private, should we actually care? It's underscored and private :)

Given that you've approved this already - I take it you'd be fine with merging this and following up with the "public API" parts later on?

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision ed96df2 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-03-06T12:56:29Z

Patchset ed96df2 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 3006935 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-03-06T13:57:27Z

Patchset 3006935 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

sqla-tester

Federico Caselli (CaselIT) wrote:

code review left on gerrit

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

alembic/util/compat.py (line 59): Done

sqla-tester · 2025-03-06T19:21:10Z

alembic/config.py

-                subparser.set_defaults(cmd=(fn, positional, kwarg))
+            )
+        )
+


Federico Caselli (CaselIT) wrote:

Done

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

sqla-tester · 2025-03-06T19:21:10Z

alembic/config.py

+        for arg in kwarg:
+            if arg in self._KWARGS_OPTS:
+                kwarg_opt = self._KWARGS_OPTS[arg]
+                args, opts = kwarg_opt[0:-1], kwarg_opt[-1]


Federico Caselli (CaselIT) wrote:

Done

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

sqla-tester · 2025-03-06T19:21:10Z

alembic/config.py

+                subparser.add_argument(*args, **opts)  # type:ignore
+
+        for arg in positional:
+            if arg in self._POSITIONAL_OPTS:


Federico Caselli (CaselIT) wrote:

Done

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5718

zzzeek

hi my comment is not linkable so reiterating here, this patch has good ideas but they need to be presented as stable API that isn't going to change

my comment is below. at the very least this needs something with public methods, perhaps a UserDefinedCommandLine subclass? with docstrings, and a bit of a test to make sure we dont accidentally change that API - otherwise the submitter here is better off vendoring their own changes locally since they are essentially private:

OK well, the movement of methods and all is fine, but the goal of the PR here means that it's not that simple.

the goal is "make it easier for third parties to subclass CommandLine". This does some initial structure for that, but it doesnt do all these other things:

document any of these methods. how would I know what _register_command does?
make it clear which methods are safe - why are these methods underscored if they are now "public" ?
provide an example in the docs how to subclass CommandLine
provide any unit tests to ensure that a user-custom CommandLine implementation continues to work, otherrwise there's nothing to prevent someone changing CommandLine again here in some arbitrary way
whether or not this is backwards compatible with someone who already did their own CommandLine._generate_args should be established also, it looks like that's maintained here

overall I like making CommandLine subclassable but that's public API, and it has to be done as a public API which means: 1. docs 2. proper conventions 3. API stability 4. tests

zzzeek · 2025-03-13T15:37:45Z

Given that you've approved this already - I take it you'd be fine with merging this and following up with the "public API" parts later on?

no, sorry, I dont know how "approve" got in there. as mentioned, this PR so far serves your immediate use case, but nobody else's since it's not documented, and there's no way to prevent someone from changing this API in the future which would break your code also without tests for API stability

zzzeek · 2025-03-13T15:38:50Z

literally, do this:

make the methods public
put a bit of a comment in each one
make a test in test_command.py where you provide the class, override the methods, and then probably use mocks to capture the arguments, make sure the mocks have expected mock.call() signatures.

that's it!

zzzeek · 2025-03-13T15:40:12Z

but also make sure the old def _generate_args is still there with that exact underscore name, so that people who have subclassed and overrode that also dont break (a simple test w/ mocks again can test this also)

Refactor CommandLine command registration

e1d6454

Fixes: sqlalchemy#1610

mikeroll force-pushed the argparse-refactor branch from 5d7b091 to e1d6454 Compare February 23, 2025 10:22

mikeroll added 2 commits February 27, 2025 15:09

fixup! Refactor CommandLine command registration

aebe80a

fixup! Refactor CommandLine command registration

369059e

zzzeek requested a review from sqla-tester March 4, 2025 17:39

sqla-tester reviewed Mar 4, 2025

View reviewed changes

zzzeek approved these changes Mar 4, 2025

View reviewed changes

sqla-tester reviewed Mar 4, 2025

View reviewed changes

fixup! Refactor CommandLine command registration

ed96df2

CaselIT requested a review from sqla-tester March 6, 2025 12:56

sqla-tester reviewed Mar 6, 2025

View reviewed changes

mikeroll added 2 commits March 6, 2025 14:30

Simplify _POSITIONAL_OPTS

16f8e85

fixup! Refactor CommandLine command registration

3006935

CaselIT requested a review from sqla-tester March 6, 2025 13:57

sqla-tester reviewed Mar 6, 2025

View reviewed changes

zzzeek requested changes Mar 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `CommandLine` command registration #1611

Refactor `CommandLine` command registration #1611

mikeroll commented Feb 23, 2025

mikeroll commented Feb 27, 2025

sqla-tester left a comment

sqla-tester commented Mar 4, 2025

zzzeek left a comment

sqla-tester left a comment

sqla-tester Mar 4, 2025

sqla-tester Mar 4, 2025

mikeroll Mar 6, 2025

sqla-tester Mar 4, 2025

mikeroll Mar 6, 2025

sqla-tester Mar 4, 2025

mikeroll Mar 6, 2025

mikeroll commented Mar 6, 2025

sqla-tester left a comment

sqla-tester commented Mar 6, 2025

sqla-tester left a comment

sqla-tester commented Mar 6, 2025

sqla-tester left a comment

sqla-tester Mar 6, 2025

sqla-tester Mar 6, 2025

sqla-tester Mar 6, 2025

zzzeek left a comment

zzzeek commented Mar 13, 2025

zzzeek commented Mar 13, 2025

zzzeek commented Mar 13, 2025

Refactor CommandLine command registration #1611

Are you sure you want to change the base?

Refactor CommandLine command registration #1611

Conversation

mikeroll commented Feb 23, 2025

Description

Checklist

mikeroll commented Feb 27, 2025

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Mar 4, 2025

zzzeek left a comment

Choose a reason for hiding this comment

sqla-tester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeroll commented Mar 6, 2025

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Mar 6, 2025

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Mar 6, 2025

sqla-tester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzzeek left a comment

Choose a reason for hiding this comment

zzzeek commented Mar 13, 2025

zzzeek commented Mar 13, 2025

zzzeek commented Mar 13, 2025

Refactor `CommandLine` command registration #1611

Refactor `CommandLine` command registration #1611