Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get_x_argument(as_dictionary=True) fails if x values doesn't have = #1369

Closed
iurisilvio opened this issue Nov 29, 2023 · 6 comments
Closed
Labels
bug Something isn't working command interface

Comments

@iurisilvio
Copy link
Contributor

iurisilvio commented Nov 29, 2023

Describe the bug

It is not safe to use get_x_argument(as_dictionary=True) because it fails with -x foo.

Expected behavior

I'd expect an empty string "" (or True? or None?) as value instead of breaking my script.

To Reproduce

from unittest.mock import Mock

from alembic.config import Config
from alembic.runtime.environment import EnvironmentContext
from alembic.script import ScriptDirectory

context = EnvironmentContext(Config(cmd_opts=Mock(x="foo")), ScriptDirectory("."))
context.get_x_argument(as_dictionary=True)

Error

$ python example.py
Traceback (most recent call last):
  File "example.py", line 8, in <module>
    context.get_x_argument(as_dictionary=True)
  File "alembic/runtime/environment.py", line 331, in get_x_argument
    value = dict(arg.split("=", 1) for arg in value)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: dictionary update sequence element #0 has length 1; 2 is required

Versions.

  • OS:
  • Python:
  • Alembic:
  • SQLAlchemy:
  • Database:
  • DBAPI:

Additional context

Have a nice day!

@iurisilvio iurisilvio added the requires triage New issue that requires categorization label Nov 29, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Nov 29, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Nov 29, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Nov 29, 2023
@CaselIT CaselIT added bug Something isn't working command interface and removed requires triage New issue that requires categorization labels Nov 29, 2023
@CaselIT
Copy link
Member

CaselIT commented Nov 29, 2023

Hi,

Thanks for reporting

I'd expect an empty string "" (or True? or None?) as value instead of breaking my script.

Yes, I think an empty string is the least surprising value here

@zzzeek
Copy link
Member

zzzeek commented Nov 29, 2023

Yes, I think an empty string is the least surprising value here

hmm not None? what do we do if someone asks for x value and there was no -x ?

@CaselIT
Copy link
Member

CaselIT commented Nov 29, 2023

Yes, I think an empty string is the least surprising value here

hmm not None? what do we do if someone asks for x value and there was no -x ?

Well if foo=bar gives me 'bar' it think the logic thing for foo= and foo is to return ''. Otherwise we have two different behaviours in that case

@zzzeek
Copy link
Member

zzzeek commented Nov 29, 2023

Yes, I think an empty string is the least surprising value here

hmm not None? what do we do if someone asks for x value and there was no -x ?

Well if foo=bar gives me 'bar' it think the logic thing for foo= and foo is to return ''. Otherwise we have two different behaviours in that case

oh ok. that's fine then

@iurisilvio
Copy link
Contributor Author

This is how I think too, foo= generate an empty string, foo should return the same value.

Talking about my use case, I just need something like multiple tags and I don't really care about the value.

I don't even need the dict. I could use "foo" in context.get_x_argument(), but then I can't use as_dictionary flag anywhere else because foo breaks it.

In any case, we still can check for the key with "foo" in context.get_x_argument(as_dictionary=True).

iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Nov 29, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Nov 30, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Dec 1, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Dec 1, 2023
…s_dictionary=True)` for args without `=`
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Dec 4, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Dec 4, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Dec 4, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Dec 4, 2023
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Dec 4, 2023
…s_dictionary=True)` for args without `=`
iurisilvio pushed a commit to iurisilvio/alembic that referenced this issue Dec 4, 2023
…ments(as_dictionary=True)` for args without `=`
@sqla-tester
Copy link
Collaborator

Iuri de Silvio has proposed a fix for this issue in the main branch:

Fix get_x_arguments(as_dictionary=True) for args without = https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4989

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working command interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants