#1069 frontend: python3-flask-script removal
Merged a month ago by frostyx. Opened a month ago by thrnciar.

@@ -69,6 +69,7 @@ 

  BuildRequires: fedora-messaging

  BuildRequires: libmodulemd < 2

  BuildRequires: libmodulemd >= 1.7.0

+ BuildRequires: python3-click

  BuildRequires: python3-CommonMark

  BuildRequires: python3-blinker

  BuildRequires: python3-copr-common > 0.4

@@ -120,6 +121,7 @@ 

  Requires: js-respond

  Requires: libmodulemd < 2

  Requires: libmodulemd >= 1.7.0

+ Requires: python3-click

  Requires: python3-CommonMark

  Requires: python3-alembic

  Requires: python3-blinker

@@ -1,37 +1,31 @@ 

  import argparse

  import os

  import subprocess

- from flask_script import Command, Option

- 

- 

- class TestCommand(Command):

- 

-     def run(self, coverage, test_args):

-         os.environ["COPRS_ENVIRON_UNITTEST"] = "1"

-         if not (("COPR_CONFIG" in os.environ) and os.environ["COPR_CONFIG"]):

-             os.environ["COPR_CONFIG"] = "/etc/copr/copr_unit_test.conf"

- 

-         if 'PYTHONPATH' in os.environ:

-             os.environ['PYTHONPATH'] = os.environ['PYTHONPATH'] + ':.'

-         else:

-             os.environ['PYTHONPATH'] = '.'

- 

-         additional_args = test_args

- 

-         if coverage:

-             additional_args.extend([

-                 '--cov-report', 'term-missing', '--cov', 'coprs'

-             ])

- 

-         return subprocess.call(["/usr/bin/python3", "-m", "pytest"] + additional_args)

- 

-     option_list = (

-         Option("-a",

-                dest="test_args",

-                nargs=argparse.REMAINDER),

-         Option("--coverage",

-                dest="coverage",

-                required=False,

-                action='store_true',

-                default=False),

-     )

+ import click

+ 

+ @click.command(context_settings=dict(

+     ignore_unknown_options=True,

+ ))

+ @click.argument("arguments", nargs=-1, type=click.UNPROCESSED)

+ @click.option("--coverage/--no-coverage",

+     default=False

+ )

+ 

+ def test(coverage, arguments):

+     os.environ["COPRS_ENVIRON_UNITTEST"] = "1"

+     if not (("COPR_CONFIG" in os.environ) and os.environ["COPR_CONFIG"]):

+         os.environ["COPR_CONFIG"] = "/etc/copr/copr_unit_test.conf"

+ 

+     if 'PYTHONPATH' in os.environ:

+         os.environ['PYTHONPATH'] = os.environ['PYTHONPATH'] + ':.'

+     else:

+         os.environ['PYTHONPATH'] = '.'

+ 

+     additional_args = list(arguments)

+ 

+     if coverage:

+         additional_args.extend([

+             '--cov-report', 'term-missing', '--cov', 'coprs'

+         ])

+ 

+     return subprocess.call(["/usr/bin/python3", "-m", "pytest"] + additional_args)

@@ -5,14 +5,14 @@ 

  import sys

  import pipes

  import importlib

+ import click

+ import commands.test

+ 

  from flask_script import Manager

  from coprs import app

  

  

- commands = {

-     # General commands

-     "test": "TestCommand",

- 

+ commands_old = {

      # Database commands

      "create_sqlite_file": "CreateSqliteFileCommand",

      "create_db": "CreateDBCommand",

@@ -57,11 +57,18 @@ 

      sys.exit(1)

  

  manager = Manager(app)

- for cmdname, clsname in commands.items():

+ for cmdname, clsname in commands_old.items():

      module = importlib.import_module("commands.{0}".format(cmdname))

      cls = getattr(module, clsname)

      manager.add_command(cmdname, cls())

  

+ app.cli.add_command(commands.test.test, "test")

  

  if __name__ == "__main__":

-     manager.run()

+     # This is just temporary while migrating to flask script,

+     # values in arrays are already migrated parameters.

+     # Else part will be removed once migration is complete.

+     if sys.argv[1] in ['test']:

+         app.cli()

+     else:

+         manager.run()

file modified
+1 -1

@@ -18,4 +18,4 @@ 

  export COPR_CONFIG="$(pwd)/coprs_frontend/config/copr_unit_test.conf"

  

  cd coprs_frontend

- ./manage.py test -a "$@"

+ ./manage.py test "$@"

commands/test.py - done

rebased onto 6940aac1b20d250a664d5335df7e8ce1a94553b2

a month ago

rebased onto c108d3c

a month ago

What is the purpose of this blank function?

Just a minor thing, but personally, I would go with import commands and then use commands.test.test later in the code. This way you don't have to care if two functions have the same name (which I don't know whether it may happen or not).

I would pick some order of those decorators and stick to it. @click.command() should go first, according to the documentation, then I would go with @click.argument because for any command, there will be only a small number of them. After that, I would put all the @click.option parameters.

Also, the -a and a is not a particularly good name :-). Previously, there was only -a and --coverage, so I am not sure what the positional a argument is for.

Thank you @thrnciar, the PR IMHO looks really good.
I made you a few notes in the code section.

Also pt2, the --help for -a is not very helpful

Options:
  -a TEXT...

can you please add some short description?

@frostyx, thank you for your feedback. I will try to explain it.

Also, the -a and a is not a particularly good name :-). Previously, there was only -a and --coverage, so I am not sure what the positional a argument is for.

The reason why the a is there is because I was trying to achieve unlimited number of arguments following the -a option. The problem was that it wasn't as easy as I firstly expected and I had to search for solution online. I have found this answer[1], that helped me to solve this problem. The reason why in my code is -a / a is because in his aswer he had --users / users. I can understand it's bit confusing. It would be probably better to ask, you guys, what was originally -a meant to be and give it some more descriptive name. So, do you remember for what is the -a option used?

[1] https://stackoverflow.com/a/54764354

rebased onto 0dbdf2c9a369a896a188dcad71d0d90a339f37ff

a month ago

Do we need the multiple-groups feature? If not, we can simply discard this code and use:

app.cli.add_command(test, "test")

and then later replace manage_new() with app.cli()

@thrnciar, does the following command work for you?

[jkadlcik@localhost frontend]$ ./run_tests.sh -k test_mail.py

I get Error: no such option: -k, which is wrong, but I am not sure whether it is PEBKAC from me.

@frostyx, are you sure, that you have test_mail.py in the same directory as ./run_tests.sh?
Shouldn't you run it as ./run_tests.sh -k coprs_frontend/tests/test_mail.py?

thanks, this is better. I'll update PR.

rebased onto a7498c88f97fc1b328328cb9ad34d8a6f9749a1c

a month ago

rebased onto b9c0a5c89f7de9e1384ac1a3deab2ad5e1ec680a

a month ago

rebased onto dacee14

a month ago

rebased onto dacee14

a month ago

Commit cf4cfc3 fixes this pull-request

Pull-Request has been merged by frostyx

a month ago