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

normalize path if it is a case insensitive file system #2421

Closed

Conversation

dongho-jung
Copy link

closes #2419(Virtualenv not always found in case insensitive filesystem)
the util func for checking whether it is a case sentive
or not is from Steve Cohen

Pull Request Check List

Resolves: #2419

  • Added tests for changed code.
  • Updated documentation for changed code.

I have no idea where to edit for documentation that I made the default behavior of EnvManager.generate_env_name normalize case of cwd. If needed, please let me know where to edit thank you.

close python-poetry#2419(Virtualenv not always found in case insensitive filesystem)
the util func for checking whether it is a case sentive
or not is from [Steve Cohen](https://stackoverflow.com/a/36580834/7899226)
@dongho-jung
Copy link
Author

dongho-jung commented May 14, 2020

I think this is unnecessary. I'm waiting for the original issuer's feedback.
this is edited, please see this

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho, the change is reasonable. I couldn't find any documentation on the naming either. Maybe, there should be a changelog entry that says that the naming of virtualenv folders for Windows users will change. Thus, an existing virtualenv is not used anymore after updating poetry but a new one is created.

At least, the test needs some improvement. See comment on concerning lines.

Comment on lines +837 to +854
def test_case_should_be_ignored_if_fs_is_case_sensitive(tmp_dir):
with tempfile.NamedTemporaryFile(prefix="TmP") as tmp_file:
if not os.path.exists(tmp_file.name.lower()):
path_with_lowercase = Path(tmp_dir) / "lowerpath"
path_with_uppercase = Path(tmp_dir) / "UPPERPATH"
path_with_bothcase = Path(tmp_dir) / "BoThCaSe"

venv_with_lowercase = EnvManager.generate_env_name(
"simple-project", str(path_with_lowercase)
)
venv_with_uppercase = EnvManager.generate_env_name(
"simple-project", str(path_with_uppercase)
)
venv_with_bothcase = EnvManager.generate_env_name(
"simple-project", str(path_with_bothcase)
)

assert venv_with_lowercase != venv_with_uppercase != venv_with_bothcase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, in its current state, the test does not do what it is supposed to do (even if the approach is fine). The test only checks if different pathes result in different venv names. (The pathes are different even if case is ignored!) The test should at least check two pathes that are equal for case insensitive file systems but different for case sensitive file systems.

Further, there are two minor quirks:

  1. The assert will be true even if venv_with_lowercase and venv_with_bothcase are equal, e.g.

    >>> 'a' != 'b' != 'a'
    True
    

    Probably, there should be three asserts each comparing a pair of venv names.

  2. The name of the test seems a little bit ambiguous to me (the meaning is "test: case should ...", but reads "'test case' should ...") and probably the wrong way around (case should be ignored for case insensitive file systems and not for case sensitive file systems).

All in all, my proposal for improvement:

Suggested change
def test_case_should_be_ignored_if_fs_is_case_sensitive(tmp_dir):
with tempfile.NamedTemporaryFile(prefix="TmP") as tmp_file:
if not os.path.exists(tmp_file.name.lower()):
path_with_lowercase = Path(tmp_dir) / "lowerpath"
path_with_uppercase = Path(tmp_dir) / "UPPERPATH"
path_with_bothcase = Path(tmp_dir) / "BoThCaSe"
venv_with_lowercase = EnvManager.generate_env_name(
"simple-project", str(path_with_lowercase)
)
venv_with_uppercase = EnvManager.generate_env_name(
"simple-project", str(path_with_uppercase)
)
venv_with_bothcase = EnvManager.generate_env_name(
"simple-project", str(path_with_bothcase)
)
assert venv_with_lowercase != venv_with_uppercase != venv_with_bothcase
def test_generate_env_name_ignores_case_for_case_insensitive_fs(tmp_dir):
path_with_lowercase = Path(tmp_dir) / "cwd"
path_with_uppercase = Path(tmp_dir) / "CWD"
path_with_bothcase = Path(tmp_dir) / "CwD"
venv_with_lowercase = EnvManager.generate_env_name(
"simple-project", str(path_with_lowercase)
)
venv_with_uppercase = EnvManager.generate_env_name(
"simple-project", str(path_with_uppercase)
)
venv_with_bothcase = EnvManager.generate_env_name(
"simple-project", str(path_with_bothcase)
)
with tempfile.NamedTemporaryFile(prefix="TmP") as tmp_file:
is_case_insensitive_fs = os.path.exists(tmp_file.name.lower())
if is_case_insensitive_fs:
assert venv_with_lowercase == venv_with_uppercase
assert venv_with_lowercase == venv_with_bothcase
assert venv_with_uppercase == venv_with_bothcase
else:
assert venv_with_lowercase != venv_with_uppercase
assert venv_with_lowercase != venv_with_bothcase
assert venv_with_uppercase != venv_with_bothcase

@radoering
Copy link
Member

@0xF4D3C0D3: May you consider to incorporate my requested changes and rebase your branch? Otherwise, if you don't mind I would like to create a new PR to make another try at getting this fix merged.

@dongho-jung
Copy link
Author

@radoering thanks for your contribution and feedback. However I think I couldn't do what you want soon. it seems to be better to close this issue and open new PR. Good luck!

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Virtualenv not always found in case insensitive filesystem
2 participants