#1069 frontend: python3-flask-script removal
Merged 10 months ago by frostyx. Opened 10 months 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 "$@"

rebased onto 6940aac1b20d250a664d5335df7e8ce1a94553b2

10 months ago

rebased onto c108d3c16a442e6f3fa5d5e84c7a77264d7d0887

10 months 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

10 months 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

10 months ago

rebased onto b9c0a5c89f7de9e1384ac1a3deab2ad5e1ec680a

10 months ago

rebased onto dacee14

10 months ago

rebased onto dacee14

10 months ago

Commit cf4cfc3 fixes this pull-request

Pull-Request has been merged by frostyx

10 months ago