alembic icon indicating copy to clipboard operation
alembic copied to clipboard

Improve handling of prepend_sys_path, fixes #1330

Open mwerezak opened this issue 2 years ago • 7 comments

Description

  • If os.name equals 'nt' then don't split using colons
  • Split on any whitespace, not just the ' ' character so people can break up long lists into multiple lines.
  • Also normalize and trim paths to prevent issues with the previous point.

Fixes: #1330

How should I test this?

Checklist

This pull request is:

  • [ ] A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • [x] A short code fix
    • please include the issue number, and create an issue if none exists, which must include a complete example of the issue. one line code fixes without an issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • [ ] A new feature implementation
    • please include the issue number, and create an issue if none exists, which must include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.

mwerezak avatar Oct 19 '23 15:10 mwerezak

Can someone please give brief guidance on how to create a test for this.

mwerezak avatar Oct 19 '23 15:10 mwerezak

If os.name equals 'nt' then don't split using colons

I would use os.pathsep for this

CaselIT avatar Oct 19 '23 16:10 CaselIT

Split on any whitespace, not just the ' ' character so people can break up long lists into multiple lines.

also wired that space is considered a separator, since a path can have a space in it.

@zzzeek do you remember the reason?

CaselIT avatar Oct 19 '23 16:10 CaselIT

Split on any whitespace, not just the ' ' character so people can break up long lists into multiple lines.

also wired that space is considered a separator, since a path can have a space in it.

@zzzeek do you remember the reason?

There was a complex legacy arrangement here when the path separator system was first introduced, so I would look at the history there to see what our default separator used to be etc.

zzzeek avatar Oct 19 '23 16:10 zzzeek

I like using os.pathsep seems much more portable.

Am I good to just remove splitting on whitespace altogether?

mwerezak avatar Oct 19 '23 16:10 mwerezak

hi there, and sorry, I looked at what we are actually doing here.

This is a new use case, and it looks like prepend_sys_path was very unfortunately glossed over when we added configurable path separators. So we have the same legacy situation where we should not have a loosely defined scheme like this.

I propose we add another new config attribute prepend_sys_path_separator and have it work in the same way as version_path_separator - if not present, then the exact broken scheme here is used for backwards compat. for new projects, the default will be "os".

Taht is like this:

# sys.path path, will be prepended to sys.path if present.
# defaults to the current working directory.  for multiple paths, the path separator
# is defined by prepend_sys_path_separator
prepend_sys_path = .

# prepend sys path separator; As mentioned above, this is the character used to split
# sys_path paths.  The default within new alembic.ini files is "os", which uses os.pathsep.
# If this key is omitted entirely, it falls back to the legacy behavior of splitting on spaces and/or commas.
# Valid values for prepend_sys_path_separator are:
#
# prepend_sys_path_separator = :
# prepend_sys_path_separator = ;
# prepend_sys_path_separator = space
prepend_sys_path_separator = os  # Use os.pathsep. Default configuration used for new projects.

zzzeek avatar Oct 19 '23 16:10 zzzeek

I replaced the earlier implementation with a new one that does what @zzzeek described. Haven't tried it yet though, will do that on Monday

mwerezak avatar Nov 04 '23 23:11 mwerezak