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

Replace spaces in platform names with underscores #620

Merged
merged 1 commit into from
Feb 11, 2023

Conversation

tucked
Copy link
Contributor

@tucked tucked commented Nov 30, 2022

Resolve #652

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Please add tests.

@tucked tucked force-pushed the platform-space branch 2 times, most recently from 46179b8 to 8297153 Compare December 3, 2022 01:12
@tucked
Copy link
Contributor Author

tucked commented Dec 3, 2022

@brettcannon done!

@brettcannon
Copy link
Member

I just realized there's no issue associated with this PR, so folks don't know there's a decision to be made here.

@tucked would you mind creating an issue?

@brettcannon
Copy link
Member

@tucked I'm ready to review this again, but I still need an issue to be created so I can discuss this with the other maintainer to make sure this isn't going to be a breaking change in some way that I can't think of.

@tucked
Copy link
Contributor Author

tucked commented Jan 3, 2023

Hey @brettcannon, thanks for getting back to me and sorry for the delay. I've been out for the holidays.

I've created #652 and linked this PR to it! I've also rebased this PR onto the latest main.

@tucked
Copy link
Contributor Author

tucked commented Jan 4, 2023

It looks like a new test was added recently which breaks with this change:

def test__generic_abi_jp(self, monkeypatch):
config = {"EXT_SUFFIX": ".return exactly this.so"}
monkeypatch.setattr(sysconfig, "get_config_var", config.__getitem__)
assert tags._generic_abi() == ["return exactly this"]

=================================== FAILURES ===================================
_____________________ TestGenericTags.test__generic_abi_jp _____________________

self = <tests.test_tags.TestGenericTags object at 0x7fc9ff9db050>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fc9fa175e10>

    def test__generic_abi_jp(self, monkeypatch):
        config = {"EXT_SUFFIX": ".return exactly this.so"}
        monkeypatch.setattr(sysconfig, "get_config_var", config.__getitem__)
>       assert tags._generic_abi() == ["return exactly this"]
E       AssertionError: assert ['return_exactly_this'] == ['return exactly this']
E         At index 0 diff: 'return_exactly_this' != 'return exactly this'
E         Full diff:
E         - ['return exactly this']
E         ?         ^       ^
E         + ['return_exactly_this']
E         ?         ^       ^

tests/test_tags.py:861: AssertionError
=========================== short test summary info ============================
FAILED tests/test_tags.py::TestGenericTags::test__generic_abi_jp - AssertionError: assert ['return_exactly_this'] == ['return exactly this']
  At index 0 diff: 'return_exactly_this' != 'return exactly this'
  Full diff:
  - ['return exactly this']
  ?         ^       ^
  + ['return_exactly_this']
  ?         ^       ^
======================= 1 failed, 26341 passed in 57.45s =======================

I suppose that suggests that we may only want to normalize spaces in _generic_platforms or maybe platform_tags:

def _generic_platforms() -> Iterator[str]:
yield _normalize_string(sysconfig.get_platform())
def platform_tags() -> Iterator[str]:
"""
Provides the platform tags for this installation.
"""
if platform.system() == "Darwin":
return mac_platforms()
elif platform.system() == "Linux":
return _linux_platforms()
else:
return _generic_platforms()

@brettcannon
Copy link
Member

I suppose that suggests that we may only want to normalize spaces in _generic_platforms or maybe platform_tags

Not necessarily. It's something we can discuss as to what's best to do.

@tucked
Copy link
Contributor Author

tucked commented Jan 26, 2023

Not necessarily. It's something we can discuss as to what's best to do.

👌 Good to know.... I already made the adjustment from the previous diff, but we can always switch back if that ends up being the best route.

@tucked
Copy link
Contributor Author

tucked commented Jan 26, 2023

Based on Matti's response in #652, I've switched back to the original diff and patched the new test.

@brettcannon brettcannon merged commit 7013a60 into pypa:main Feb 11, 2023
@brettcannon
Copy link
Member

@tucked Thanks!

@tucked tucked deleted the platform-space branch February 11, 2023 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spaces in platform names are problematic
3 participants