Skip to content

Commit

Permalink
[FIX] odev: parsing of unknown arguments in python 3.12.7+
Browse files Browse the repository at this point in the history 
Breaking change: passing unknown (odoo-bin) arguments before positional
arguments is not supported in Python >= 3.12.7. In the same idea, it is
also not supported anymore to pass known arguments after positional
arguments.

It is not planned to reimplement this feature in the later versions
as it was taking advantage of a bug not compliant with common CLI
rulings and RFCs.

When in doubt, please refer to the command's help and follow
the proposed order for arguments.

See: python/cpython#59317
  • Loading branch information
@brinkflew
brinkflew committed Oct 31, 2024
1 parent 5df1343 commit 3d031e7
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 19 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/odev.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ jobs:
runs-on: ${{ matrix.os }}-latest

strategy:
fail-fast: true
fail-fast: false
matrix:
os:
- ubuntu
Expand Down
12 changes: 10 additions & 2 deletions odev/common/commands/base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,11 @@ def prepare_arguments(cls, parser: ArgumentParser) -> None:

if params.get("nargs") == "*...":
cls._unknown_arguments_dest = aliases[0]
params["nargs"] = "*"

# A bug in standard library argparse before python 3.12.7 causes `...` to not work as expected
# when used in conjunction with positional and optional arguments.
# See: https://github.com/python/cpython/issues/59317
params["nargs"] = "*" if sys.version_info < (3, 12, 7) else "..."

if "action" in params:
params["action"] = ACTIONS_MAPPING.get(params["action"], params["action"])
Expand Down Expand Up @@ -281,7 +285,11 @@ def parse_arguments(cls, argv: Sequence[str]) -> Namespace:
arguments = parser.parse_args(argv)
else:
arguments, unknown = parser.parse_known_args(argv)
setattr(arguments, cls._unknown_arguments_dest, unknown)
setattr(
arguments,
cls._unknown_arguments_dest,
getattr(arguments, cls._unknown_arguments_dest, []) + unknown,
)
except SystemExit as exception:
error_message = stderr.getvalue()
error = re.search(rf"{cls._name}: error: (.*)$", error_message, re.MULTILINE)
Expand Down
1 change: 0 additions & 1 deletion odev/common/commands/odoobin.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,6 @@ class OdoobinTemplateCommand(OdoobinCommand):
from_template = args.String(
aliases=["-t", "--from-template"],
description="Name of an existing PostgreSQL database to copy.",
nargs="?",
default="",
)

Expand Down
75 changes: 60 additions & 15 deletions tests/commands/test_database.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def test_01_cloc(self):
def test_02_cloc_csv(self):
"""Command `odev cloc --csv` should display line of codes count by module in csv format."""
with self.patch(ODOOBIN_PATH, "run", CompletedProcess(args=["cloc"], returncode=0, stdout=self.CLOC_RESULT)):
stdout, _ = self.dispatch_command("cloc", self.database.name, "--csv")
stdout, _ = self.dispatch_command("cloc", "--csv", self.database.name)

self.assertIn("test_module_01", stdout)
self.assertEqual(len(stdout.strip().splitlines()), 6)
Expand Down Expand Up @@ -96,7 +96,7 @@ def test_01_create_bare(self):
self.assert_database(self.create_database_name, exists=False)

with self.wrap("odev.common.bash", "stream") as stream:
self.dispatch_command("create", self.create_database_name, "--bare")
self.dispatch_command("create", "--bare", self.create_database_name)
stream.assert_not_called()

self.assert_database(self.create_database_name, exists=True, is_odoo=False)
Expand All @@ -107,7 +107,12 @@ def test_02_create_odoo(self):

with self.wrap("odev.common.bash", "stream") as stream:
stdout, _ = self.dispatch_command(
"create", self.create_database_name, "--version", "17.0", "--without-demo", "all"
"create",
"--version",
"17.0",
self.create_database_name,
"--without-demo",
"all",
)
stream.assert_called_with(
OdoobinMatch(
Expand All @@ -126,7 +131,12 @@ def test_03_create_from_template(self):
self.assert_database(self.create_database_name, exists=False)

with self.wrap("odev.common.bash", "stream") as stream:
self.dispatch_command("create", self.create_database_name, "--from-template", template_database.name)
self.dispatch_command(
"create",
"--from-template",
template_database.name,
self.create_database_name,
)
stream.assert_not_called()

self.assert_database(self.create_database_name, version=str(template_database.version))
Expand All @@ -138,7 +148,7 @@ def test_04_create_new_template(self):
self.assert_database(self.template_database_name, exists=False)

with self.wrap("odev.common.bash", "stream") as stream:
self.dispatch_command("create", self.create_database_name, "--create-template")
self.dispatch_command("create", "--create-template", self.create_database_name)
stream.assert_not_called()

self.assert_database(self.template_database_name, version=str(database.version))
Expand All @@ -151,7 +161,13 @@ def test_05_overwrite(self):

with self.wrap("odev.common.bash", "stream") as stream:
stdout, _ = self.dispatch_command(
"create", self.create_database_name, "--force", "--version", "16.0", "--without-demo", "all"
"create",
"--force",
"--version",
"16.0",
self.create_database_name,
"--without-demo",
"all",
)
stream.assert_called_with(
OdoobinMatch(
Expand All @@ -168,15 +184,29 @@ def test_06_with_venv(self):
"""Command `odev create` should create a new database with a specific virtual environment."""
self.assert_database(self.create_database_name, is_odoo=False)
self.dispatch_command(
"create", self.create_database_name, "--venv", self.venv.name, "--version", "17.0", "--without-demo", "all"
"create",
"--venv",
self.venv.name,
"--version",
"17.0",
self.create_database_name,
"--without-demo",
"all",
)
self.assert_database(self.create_database_name, is_odoo=True)
self.assertEqual(LocalDatabase(self.create_database_name).venv.name, self.venv.name)

def test_07_with_version(self):
"""Command `odev create` should create a new database with a specific Odoo version."""
self.assert_database(self.create_database_name, is_odoo=False)
self.dispatch_command("create", self.create_database_name, "--version", "17.0", "--without-demo", "all")
self.dispatch_command(
"create",
"--version",
"17.0",
self.create_database_name,
"--without-demo",
"all",
)
self.assert_database(self.create_database_name, is_odoo=True, version="17.0")
database = LocalDatabase(self.create_database_name)
self.assertEqual(database.venv.name, "17.0")
Expand All @@ -186,7 +216,14 @@ def test_08_with_worktree(self):
"""Command `odev create` should create a new database with a specific worktree."""
self.assert_database(self.create_database_name, is_odoo=False)
self.dispatch_command(
"create", self.create_database_name, "--worktree", "test", "--version", "17.0", "--without-demo", "all"
"create",
"--worktree",
"test",
"--version",
"17.0",
self.create_database_name,
"--without-demo",
"all",
)
self.assert_database(self.create_database_name, is_odoo=True)
database = LocalDatabase(self.create_database_name)
Expand All @@ -200,7 +237,7 @@ class TestCommandDatabaseTest(OdevCommandTestCase):
def test_01_test(self):
"""Command `odev test` should run tests on a database."""
with self.wrap("odev.common.bash", "stream") as stream:
stdout, _ = self.dispatch_command("test", self.database.name, "--tags", ":TestSafeEval.test_expr")
stdout, _ = self.dispatch_command("test", "--tags", ":TestSafeEval.test_expr", self.database.name)
stream.assert_called_with(
OdoobinMatch(
self.database.name,
Expand Down Expand Up @@ -278,15 +315,19 @@ def test_01_run(self):
def test_02_run_from_template(self):
"""Command `odev run` should run Odoo in a database from a template."""
self.assert_database(self.database.name, is_odoo=True)
self.dispatch_command("create", self.database.name, "--create-template")
self.dispatch_command("create", "--create-template", self.database.name)
template_name = f"{self.database.name}:template"
self.assert_database(template_name, is_odoo=True)
self.database.query("CREATE TABLE test_table (id SERIAL PRIMARY KEY);")
self.assertTrue(self.database.table_exists("test_table"))

with self.wrap("odev.common.bash", "stream") as stream:
stdout, _ = self.dispatch_command(
"run", self.database.name, "--stop-after-init", "--from-template", template_name
"run",
"--from-template",
template_name,
self.database.name,
"--stop-after-init",
)
stream.assert_called_with(OdoobinMatch(self.database.name, ["--stop-after-init"]))

Expand All @@ -302,13 +343,13 @@ def test_03_run_from_template_no_value(self):
self.assert_database(self.database.name, is_odoo=True)
template_name = f"{self.database.name}:template"
self.assert_database(template_name, exists=False)
self.dispatch_command("create", self.database.name, "--create-template")
self.dispatch_command("create", "--create-template", self.database.name)
self.assert_database(template_name, is_odoo=True)
self.database.query("CREATE TABLE test_table (id SERIAL PRIMARY KEY);")
self.assertTrue(self.database.table_exists("test_table"))

with self.wrap("odev.common.bash", "stream") as stream:
stdout, _ = self.dispatch_command("run", self.database.name, "--stop-after-init", "--from-template")
stdout, _ = self.dispatch_command("run", "--from-template", "", self.database.name, "--stop-after-init")
stream.assert_called_with(OdoobinMatch(self.database.name, ["--stop-after-init"]))

self.assert_database(self.database.name, is_odoo=True)
Expand All @@ -324,7 +365,11 @@ def test_04_run_from_invalid_template(self):
self.database.query("CREATE TABLE test_table (id SERIAL PRIMARY KEY);")
self.assertTrue(self.database.table_exists("test_table"))
stdout, _ = self.dispatch_command(
"run", self.database.name, "--stop-after-init", "--from-template", template_name
"run",
"--from-template",
template_name,
self.database.name,
"--stop-after-init",
)
self.assert_database(self.database.name, is_odoo=True)
self.assertTrue(self.database.table_exists("test_table"))
Expand Down

0 comments on commit 3d031e7

Please sign in to comment.