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

[Feature-Request] Support PathLike interface in directory parameter #318

Closed
nioncode opened this issue Mar 10, 2020 · 7 comments · Fixed by #319 or eddieferrer/sizesquirrel-open#216
Assignees

Comments

@nioncode
Copy link
Contributor

It would be great if the constructor of Migrate would accept a PathLike object instead of requiring a string.

E.g., currently i have to use:

Migrate(app, db, directory="db-migrations")

It would be nice to be able to do:

Migrate(app, db, directory=Path("db-migrations"))

In this example, it does not really matter, but in my application I source all paths from a config object that uses Path for all file system paths and Flask-Migrate is the only one not supporting this and I have to use str(Path) to convert my Path back to a str.

This is probably pretty simple to implement, because the restriction seems to come from the line config.set_main_option('script_location', directory) which could simply call str(directory) instead, at least that is what I have in my stacktrace:

Traceback (most recent call last):
  File "/proj/backend/venv/bin/flask", line 8, in <module>
    sys.exit(main())
  File "/proj/backend/venv/lib/python3.6/site-packages/flask/cli.py", line 966, in main
    cli.main(prog_name="python -m flask" if as_module else None)
  File "/proj/backend/venv/lib/python3.6/site-packages/flask/cli.py", line 586, in main
    return super(FlaskGroup, self).main(*args, **kwargs)
  File "/proj/backend/venv/lib/python3.6/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/proj/backend/venv/lib/python3.6/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/proj/backend/venv/lib/python3.6/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/proj/backend/venv/lib/python3.6/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/proj/backend/venv/lib/python3.6/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/proj/backend/venv/lib/python3.6/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/proj/backend/venv/lib/python3.6/site-packages/flask/cli.py", line 426, in decorator
    return __ctx.invoke(f, *args, **kwargs)
  File "/proj/backend/venv/lib/python3.6/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/proj/backend/venv/lib/python3.6/site-packages/flask_migrate/cli.py", line 31, in init
    _init(directory, multidb)
  File "/proj/backend/venv/lib/python3.6/site-packages/flask_migrate/__init__.py", line 95, in wrapped
    f(*args, **kwargs)
  File "/proj/backend/venv/lib/python3.6/site-packages/flask_migrate/__init__.py", line 127, in init
    config.set_main_option('script_location', directory)
  File "/proj/backend/venv/lib/python3.6/site-packages/alembic/config.py", line 242, in set_main_option
    self.set_section_option(self.config_ini_section, name, value)
  File "/proj/backend/venv/lib/python3.6/site-packages/alembic/config.py", line 269, in set_section_option
    self.file_config.set(section, name, value)
  File "/nix/store/p9ikwbfdfkp04i4hz00bf4fd0km1vds2-python3-3.6.9/lib/python3.6/configparser.py", line 1192, in set
    self._validate_value_types(option=option, value=value)
  File "/nix/store/p9ikwbfdfkp04i4hz00bf4fd0km1vds2-python3-3.6.9/lib/python3.6/configparser.py", line 1177, in _validate_value_types
    raise TypeError("option values must be strings")
TypeError: option values must be strings
@miguelgrinberg
Copy link
Owner

Would you like to have a go at this? Happy to accept a PR with this change.

@nioncode
Copy link
Contributor Author

Sure, thanks for the fast feedback. I'll try to draft this up by end of the week at the latest.

@nioncode
Copy link
Contributor Author

Just wondering, what is the minimum required Python version of Flask-Migrate? Do I have to take care of being backwards compatible on Python < 3.6 (introduction of PathLike) or Python < 3.4 (introduction of pathlib)?

@miguelgrinberg
Copy link
Owner

Do you need to refer directly to Path and/or PathLike? I'd suggest you just do str(path) without requiring the path to be a specific type.

@nioncode
Copy link
Contributor Author

That would be the straight-forward thing to do, but it would except basically all python objects, since they all provide a str() implementation. I was thinking of using os.fspath() and catching AttributeError (for when os.fspath does not exist, i.e. Python < 3.6) and TypeError (for when os.fspath raises it, because the passed object does not implement __fspath__).

The TypeError I'd like to re-raise and the AttributeError I'd like to ignore and then use the object as it is done currently (i.e. simply pass whatever object was passed further) to keep backwards compatibilty.

Is this too cumbersome and I should just go with the str(path) implementation?

@miguelgrinberg
Copy link
Owner

I actually see it as a good feature to have that any objects can be used with the only requirement that they return the path when str() is applied to them.

@nioncode
Copy link
Contributor Author

Alright, then I'll implement it with just calling str() 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants