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

Improve handling of prepend_sys_path, fixes #1330 #1331

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mwerezak
Copy link

@mwerezak mwerezak commented Oct 19, 2023

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
  • 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.

Don't split using colons on windows
Fixes sqlalchemy#1330
@mwerezak
Copy link
Author

mwerezak commented Oct 19, 2023

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

@CaselIT
Copy link
Member

CaselIT commented Oct 19, 2023

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

I would use os.pathsep for this

@CaselIT
Copy link
Member

CaselIT commented Oct 19, 2023

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?

@zzzeek
Copy link
Member

zzzeek commented Oct 19, 2023

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.

@mwerezak
Copy link
Author

I like using os.pathsep seems much more portable.

Am I good to just remove splitting on whitespace altogether?

@zzzeek
Copy link
Member

zzzeek commented Oct 19, 2023

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.

@mwerezak
Copy link
Author

mwerezak commented Nov 4, 2023

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

@zzzeek
Copy link
Member

zzzeek commented Nov 7, 2024

this would need some tests to move along. it's a year later so I'd have to look closely again at this to see what we're doing (sorry I dont have a good memory)

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

Successfully merging this pull request may close these issues.

prepend_sys_path does not work with absolute paths on windows
3 participants