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

Fix _legalize_path types #5224

Open
wants to merge 2 commits into
base: fix-most-types-in-util-init
Choose a base branch
from

Conversation

snejus
Copy link
Member

@snejus snejus commented May 4, 2024

Description

Part 2 of the work fixing types in beets.util.__init__ #5215.

Mypy was not happy here because _legalize_stage function implementation concatenates path and extension parameters, implying that their types need to match.

You can see that initially path parameter was defined as a str while extension was bytes.

In reality, depending on the fragment parameter value, extension was sometimes provided as a str and sometimes as bytes. The same parameter decided whether path gets converted into bytes within _legalize_stage implementation. No surprise that mypy was confused here.

_legalize_stage is only used within Item.destination method implementation which is where fragment is defined. I determined that the fragment parameter controls the form of the output path:

  • fragment=False returned absolute path as bytes (default)
  • fragment=True returned path relative to the library directory as str.

Given the above, the change

  1. Renames fragment parameter to relative_to_libdir for clarity

  2. Makes Item.destination to return the same type in both cases. I picked bytes since that's the type that majority of the code using this method expects.

    I converted the output path to str for the code that has been expecting a string there.

  3. Decouples _legalize_stage and _legalize_path implementations from the relative_to_libdir. The logic now uses str type only.

snejus added 2 commits May 4, 2024 13:04
Mypy was not happy here because `_legalize_stage` function
implementation concatenates `path` and `extension` parameters, implying
that their types need to match.

You can see that initially `path` parameter was defined as a `str` while
`extension` was `bytes`.

In reality, depending on the `fragment` parameter value, `extension` was
sometimes provided as a `str` and sometimes as `bytes`. The same
parameter decided whether `path` gets converted into `bytes` within
`_legalize_stage` implementation. No surprise that mypy was confused
here.

`_legalize_stage` is only used within `Item.destination` method
implementation which accepts where `fragment` is defined. I determined
that the `fragment` parameter controls the form of the output path:

- fragment=False returned path absolute path *as bytes* (default)
- fragment=True returned path relative to the library directory as *str*.

Given the above, this commit

1. Renames `fragment` parameter to `relative_to_libdir` for clarity
2. Makes `Item.destination` to return the same type in both cases.
   I picked `bytes` since that's the type that majority of the code
   using this method expects.

   I converted the output path to str for the code that has been
   expecting a string here.
3. Decouples `_legalize_stage` and `_legalize_path` implementations from
   the `relative_to_libdir`. The logic now uses `str` type only.
@snejus snejus self-assigned this May 4, 2024
@snejus snejus requested a review from wisp3rwind May 4, 2024 12:35

This comment was marked as resolved.

@snejus snejus force-pushed the fix-most-types-in-util-init branch 4 times, most recently from 79cf3fd to 6a1f684 Compare May 6, 2024 08:30
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.

None yet

1 participant