PR#1094: frontend: refactorization of command in manage.py - copr/copr

copr / copr

#1094 frontend: refactorization of command in manage.py

Merged 4 years ago by praiskup. Opened 4 years ago by thrnciar.

copr/ thrnciar/copr python3-flask-script-removal-refactorization into master

frontend: refactorization of command in manage.py

Tomas Hrnciar • 4 years ago

68b83f1

frontend/coprs_frontend/commands/create_chroot.py

file modified

+3 -3

		`@@ -21,7 +21,7 @@`
		`print("{0} - chroot doesn\"t exist.".format(chroot_name))`


		`- def create_chroot(chroot_names, branch=None, activated=True):`
		`+ def create_chroot_function(chroot_names, branch=None, activated=True):`
		`"""Creates a mock chroot in DB"""`
		`for chroot_name in chroot_names:`
		`if not branch:`
		`@@ -53,6 +53,6 @@`
		help="Activate the chroot later, manually by `alter_chroot`",
		`default=True`
		`)`
		`- def create_chroot_command(chroot_names, branch=None, activated=True):`
		`+ def create_chroot(chroot_names, branch=None, activated=True):`
		`"""Creates a mock chroot in DB"""`
		`- return create_chroot(chroot_names, branch, activated)`
		`+ return create_chroot_function(chroot_names, branch, activated)`

frontend/coprs_frontend/commands/create_db.py

file modified

+2 -2

		`@@ -1,6 +1,6 @@`
		`from coprs import db`
		`from coprs.logic import builds_logic`
		`- from commands.create_sqlite_file import create_sqlite_file`
		`+ from commands.create_sqlite_file import create_sqlite_file_function`
		`import click`


		`@@ -14,7 +14,7 @@`
		`required=True`
		`)`
		`def create_db(alembic_ini):`
		`- create_sqlite_file()`
		`+ create_sqlite_file_function()`
		`db.create_all()`
		`# load the Alembic configuration and generate the`
		`# version table, "stamping" it with the most recent rev:`

frontend/coprs_frontend/commands/create_sqlite_file.py

file modified

+2 -2

		`@@ -8,7 +8,7 @@`
		`Used for alembic, "create_db" does this automatically.`
		`"""`

		`- def create_sqlite_file():`
		`+ def create_sqlite_file_function():`
		`if flask.current_app.config["SQLALCHEMY_DATABASE_URI"].startswith("sqlite"):`
		`# strip sqlite:///`
		`datadir_name = os.path.dirname(`
		`@@ -17,5 +17,5 @@`
		`os.makedirs(datadir_name)`

		`@click.command()`
		`- def create_sqlite_file_command():`
		`+ def create_sqlite_file():`
		`return create_sqlite_file()`

frontend/coprs_frontend/manage.py

file modified

+39 -33

		`@@ -43,44 +43,50 @@`
		`' '.join([pipes.quote(arg) for arg in sys.argv])))`
		`sys.exit(1)`

		`- # General commands`
		`- app.cli.add_command(commands.runserver.runserver, "runserver")`
		`- app.cli.add_command(commands.test.test, "test")`
		`+ commands_list = [`
		`+ # General commands`
		`+ "runserver",`
		`+ "test",`

		`- # Database commands`
		`- app.cli.add_command(commands.create_sqlite_file.create_sqlite_file_command, "create_sqlite_file")`
		`- app.cli.add_command(commands.create_db.create_db, "create_db")`
		`- app.cli.add_command(commands.drop_db.drop_db, "drop_db")`
		`+ # Database commands`
		`+ "create_sqlite_file",`
		`+ "create_db",`
		`+ "drop_db",`

		`- # Chroot commands`
		`- app.cli.add_command(commands.create_chroot.create_chroot_command, "create_chroot")`
		`- app.cli.add_command(commands.alter_chroot.alter_chroot, "alter_chroot")`
		`- app.cli.add_command(commands.display_chroots.display_chroots, "display_chroots")`
		`- app.cli.add_command(commands.drop_chroot.drop_chroot, "drop_chroot")`
		`- app.cli.add_command(commands.branch_fedora.branch_fedora, "branch_fedora")`
		`- app.cli.add_command(commands.comment_chroot.comment_chroot, "comment_chroot")`
		`+ # Chroot commands`
		`+ "create_chroot",`
		`+ "alter_chroot",`
		`+ "display_chroots",`
		`+ "drop_chroot",`
		`+ "branch_fedora",`
		`+ "comment_chroot",`

		`- # User commands`
		`- app.cli.add_command(commands.alter_user.alter_user, "alter_user")`
		`- app.cli.add_command(commands.add_user.add_user, "add_user")`
		`- app.cli.add_command(commands.dump_user.dump_user, "dump_user")`
		`+ # User commands`
		`+ "alter_user",`
		`+ "add_user",`
		`+ "dump_user",`

		`- # Whooshee indexes`
		`- app.cli.add_command(commands.update_indexes.update_indexes, "update_indexes")`
		`- app.cli.add_command(commands.update_indexes_quick.update_indexes_quick, "update_indexes_quick")`
		`- app.cli.add_command(commands.update_indexes_required.update_indexes_required, "update_indexes_required")`
		`+ # Whooshee indexes`
		`+ "update_indexes",`
		`+ "update_indexes_quick",`
		`+ "update_indexes_required",`

		`- # Other`
		`- app.cli.add_command(commands.get_admins.get_admins, "get_admins")`
		`- app.cli.add_command(commands.fail_build.fail_build, "fail_build")`
		`- app.cli.add_command(commands.rawhide_to_release.rawhide_to_release, "rawhide_to_release")`
		`- app.cli.add_command(commands.update_graphs.update_graphs, "update_graphs")`
		`- app.cli.add_command(commands.vacuum_graphs.vacuum_graphs, "vacuum_graphs")`
		`- app.cli.add_command(commands.notify_outdated_chroots.notify_outdated_chroots, "notify_outdated_chroots")`
		`- app.cli.add_command(commands.delete_outdated_chroots.delete_outdated_chroots, "delete_outdated_chroots")`
		`- app.cli.add_command(commands.clean_expired_projects.clean_expired_projects, "clean_expired_projects")`
		`- app.cli.add_command(commands.clean_old_builds.clean_old_builds, "clean_old_builds")`
		`- app.cli.add_command(commands.delete_orphans.delete_orphans, "delete_orphans")`
		`+ # Other`
		`+ "get_admins",`
		`+ "fail_build",`
		`+ "rawhide_to_release",`
		`+ "update_graphs",`
		`+ "vacuum_graphs",`
		`+ "notify_outdated_chroots",`
		`+ "delete_outdated_chroots",`
		`+ "clean_expired_projects",`
		`+ "clean_old_builds",`
		`+ "delete_orphans",`
		`+ ]`
		`+`
		`+ for command in commands_list:`
		`+ command_func = getattr(getattr(commands, command), command)`
		`+ app.cli.add_command(command_func)`

		`if __name__ == "__main__":`
		`app.cli()`

no initial comment

praiskup commented 4 years ago

Can you please rebase on top of current master, to fix CI?

praiskup commented 4 years ago

I in particular used to use ./manage.py runserver. Is there some alternative now with click?

praiskup commented on line 79 of frontend/coprs_frontend/manage.py 4 years ago

While I don't have big problem with this new approach .... I don't see
anything wrong with the expanded form
(the app.cli.add_command(commands.delete.....) form).

IOW, the decision to split the commands to multiple files has been done
long time ago, so we need to do some work to glue things together. The
shortened version will be harder to grep, etc.

rebased onto 17a876f25a083e19301808fe108ef3e1afc806aa

4 years ago

praiskup commented 4 years ago

frostyx commented on line 79 of frontend/coprs_frontend/manage.py 4 years ago

IOW, the decision to split the commands to multiple files has been done
long time ago, so we need to do some work to glue things together.

But I would still say, that it was for the better. We have quite a lot of logic in our manage.py code and having it all in one file was IMHO a big mess.

anything wrong with the expanded form

Well, it requires us to read/duplicate a ton of characters while we are interested only in command names. In the non-refactored version, I just can't even see them.

However, I am not saying we need to do it this way. If you have any other preferences, let me know. I just find this version much more readable, than the original (even with the cost of that double-getattr line).

praiskup commented 4 years ago

We discussed later with tomáš that we should probably drop the manage.py entirely (one day, not necessary immediately) and transform it into toy thin wrapper around /bin/flask-3 binary. And then use the flask-suggested way of adding commands to flask-3 (aka @app.cli.command decorator, see https://flask.palletsprojects.com/en/1.1.x/cli/#custom-commands).

IOW, I don't mind now ... Pick what you prefer in this PR.

praiskup commented 4 years ago