version_locations doesn't support directories with spaces (e.g. C:\Program Files\MyApp)

[marcinbarczynski]

Describe the bug

Space is used a path separator in version_locations. As a result, it's not possible to set custom version_locations to a directory that contains spaces (e.g. C:\Program Files\MyApp)

Expected behavior

Directories with spaces are supported.

To Reproduce

>>> from alembic.config import Config 
>>> from alembic.script import ScriptDirectory

>>> c = Config("C:\\Program Files\\MyApp\\alembic.ini")   
>>> c.set_main_option("version_locations", "C:\\Program Files\\MyApp\\MyVersions")
>>> s = ScriptDirectory.from_config(c)
>>> s.version_locations  
['C:\\Program', 'Files\\MyApp\\MyVersions']

Error

Buggy line: https://github.com/sqlalchemy/alembic/blob/rel_1_6_2/alembic/script/base.py#L140

Versions.

All

Additional context

Have a nice day!

[zzzeek]

resolution proposal

1. introduce new config variable "version_path_separator". This config value may have the following values:

# colon
version_path_separator = :

# semicolon
version_path_separator = ;

# use the value of os.pathsep; colon for unix, semicolon for windows
version_path_separator = os

# use a space character
version_path_separator = space

2. if the value is outside of those, raise an error, rather than silently failing / ignoring

3. within each alembic.ini.mako, add version_path_separator = os , so that new installations will default to using the os.path separator

4. update the sample version location doc:

# version location specification; this defaults
# to ${script_location}/versions.  When using multiple version
# directories, initial revisions must be specified with --version-path.
# the path separator used here should be the separator specified by "version_path_separator"
# version_locations = %(here)s/bar:%(here)s/bat:${script_location}/versions

5. for a config where version_path_separator is totally absent from the config, the default value is the space character for backwards compat concerns

6. tests added in tests/test_script_production.py -> NormPathTest, likely allowing _multi_dir_testing_config to accept additional pathseps

[CaselIT]

If instead we use a \ as the escape for the space, like the linux shell?

[CaselIT]

If instead we use a \ as the escape for the space, like the linux shell?

I guess the issue is that it's a bit strange to express for windows like paths:

win_path = "C:\\Program\\ Files\\MyApp\\MyVersions"
posix_path = "/foo/bar\\ bar/versions"

[zzzeek]

yeah it's not going to work with straight up windows paths and neither is colon as a pathsep

[gordthompson]

If the user wants to pass a version_locations path that includes spaces, why not just pass it/them as a list?

def parse_version_locations(v_l):
    if isinstance(v_l, (list, tuple, set)):
        return v_l
    else:
        # simplified version of current approach
        return v_l.split(" ")

print(parse_version_locations("/foo /bar"))
# ['/foo', '/bar']
# ok (as currently implemented)

print(parse_version_locations(r"C:\Program Files\foo"))
# ['C:\\Program', 'Files\\foo']
# no es bueno

print(parse_version_locations([r"C:\Program Files\foo"]))
# ['C:\\Program Files\\foo']
# ok

print(parse_version_locations([r"C:\Program Files\foo", r"C:\bar"]))
# ['C:\\Program Files\\foo', 'C:\\bar']
# ok

[zzzeek]

what does one put in the alembic.ini file for those? i thought that was the issue here.

[gordthompson]

The repro code in the issue report uses

c.set_main_option("version_locations", "C:\\Program Files\\MyApp\\MyVersions")

so I was suggesting that it be

c.set_main_option("version_locations", ["C:\\Program Files\\MyApp\\MyVersions"])

with the corresponding changes in the code that parses it, of course.

[zzzeek]

OK so yes we can have set_main_option accept a list, if it doesn't already. i guess the poster didn't indicate "how do i put this in my alembic.ini file", but I think we should address that also. hows that?

[gordthompson]

Sure. A list in the .ini file would just be the string JSON representation:

"""alembic.ini contains:
[alembic]
version_locations=["C:\\Program Files\\foo"]
"""

config = configparser.ConfigParser()
config.read(r"C:\Users\Gord\Desktop\alembic.ini")
ini_string_value = config["alembic"]["version_locations"]
print(ini_string_value)  # ["C:\\Program Files\\foo"]
try:
    value_to_parse = json.loads(ini_string_value)
except json.decoder.JSONDecodeError:
    print("(not valid JSON - use literal)")
    value_to_parse = ini_string_value
print(parse_version_locations(value_to_parse))
# ['C:\\Program Files\\foo']

[sqla-tester]

Gord Thompson has proposed a fix for this issue in the master branch:

Support version_locations paths with spaces https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2869

[zzzeek]

IMO I don't think json is the most traditional approach for .ini file values, particularly paths as that's what os.pathsep is for. I think sticking with os.pathsep with options to customize for backwards compatibility will be a simpler approach with better backwards compatibility and easier to understand.