Skip to content

Transition to pathlib#3704

Closed
jtpavlock wants to merge 1 commit intomasterfrom
pathlib
Closed

Transition to pathlib#3704
jtpavlock wants to merge 1 commit intomasterfrom
pathlib

Conversation

@jtpavlock
Copy link
Copy Markdown
Contributor

@jtpavlock jtpavlock commented Jul 31, 2020

Description

Works towards implementing pathlib everywhere #1409

An in progress pathlib implementation. My goal for this is to incrementally transition modules/plugins to using pathlib and the new path module over current methods. I don't expect pathlib to be used "everywhere" by the time this PR is able to be merged. This code should live alongside the existing path logic until all modules are fully transitioned.

The idea is also to create separate pull requests for each transition case to try and split up the review process. This branch should serve as a base for pathlib related code that has already been reviewed.

Implementation/Design Notes:

  • I initially thought of just subclassing pathlib.Path and pathlib.PurePath to extend for relevant beets logic. This actually isn't easy(/possible?) due to the complex relationships inside the pathlib module. https://bugs.python.org/issue24132
  • My current plan is to pass pathlib.PurePath objects around everywhere, converting to pathlib.Path objects if actual file operations are needed, and converting the objects to and from bytes for the database

To Do

  • Proof of concept - transition first module/plugin to pathlib and thoroughly test
  • Documentation - advise new convention to use the path module and pathlib over current methods
  • Tests

This pull request can be merged once python 2.7, 3.4, and 3.5 support are dropped (with v1.5.0) and the above checks are completed.

@jtpavlock
Copy link
Copy Markdown
Contributor Author

jtpavlock commented Jul 31, 2020

I'd like to continue design discussion here.

One thing I still would like to come back to is long Windows paths. Starting with Python 3.6, long paths are supported once you enable the option on Windows. I don't think it's worth the trouble of hacking this, especially given the difficulties of subclassing pathlib.

Good documentation should go a long way here, and according to https://bugs.python.org/issue27731, python will throw an exception if you try to create a path >260 (without the windows flag set), which we can always try to work with if the docs aren't sufficient.

Interestingly, the long_path flag is already enabled for me on fairly fresh Windows install (I know I didn't explicitly enable it myself), and I can confirm the long path behavior with pathlib works.

Perhaps a reasonable solution is to not implement it, and then if it seems to still be a necessary feature (for whatever reason), then assess from there as a feature request.

@jtpavlock
Copy link
Copy Markdown
Contributor Author

jtpavlock commented Jul 31, 2020

Also, since I'm doing an incremental transition to pathlib, I'd like to create some tests that assert the current path value received from the library (from an item or album) is the same as if I call pathlib.PurePath on it.

In essence, my idea is to test something like assert item.path == bytes(pathlib.PurePath(os.fsdecode(item.path))) so we know if pathlib is changing anything on us.

Two questions:

  1. Is this the right idea for that test, and where's the best place to test this? Should i do it at the item level as I described above? Or should it be on something lower like library.PathType's from_sql method (note: I don't have an intimate knowledge of the database and how everything works so I'm hoping you can help fill in the gaps where needed 😄 )
  2. What are some good test cases for this test? I imagine you have a better understanding of where the edge cases might pop up than I do.

Edit: this is a great article on pathlib and dealing with bytes

@arogl arogl mentioned this pull request Aug 1, 2020
1 task
@michaeltoohig
Copy link
Copy Markdown

How is progress coming along? I'd like to contribute if I have the free time. Is there a branch I can checkout for this migration?

@stale

This comment has been minimized.

1 similar comment
@stale

This comment has been minimized.

@stale stale bot added the stale label Jul 6, 2021
@jtpavlock
Copy link
Copy Markdown
Contributor Author

jtpavlock commented Jul 6, 2021

It's been a while since I've looked at this, and I don't currently have any plans for continuing this. IIRC, I had a couple of issues implementing this

  1. This change touches code that isn't covered by tests, and I was unsure if I was breaking code or not because of it.
  2. I was unable to come up with a good way of slowly or incrementally transitioning the codebase to pathlib. Fully shifting beets from it's current path implementation to pathlib requires changing a lot of code all at once, and I had trouble maintaining confidence during the transition that I was properly implementing the change and that it would work out in the end.

I'll close this PR, but hopefully these thoughts and the discussion in #1409 can be helpful for anyone that wants to attempt to make the transition in the future.

@jtpavlock jtpavlock closed this Jul 6, 2021
@snejus snejus deleted the pathlib branch June 15, 2024 03:48
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.

2 participants