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.

Sign up for GitHub

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

Jump to bottom

Add option to force forward slashes in paths #3334

Merged
merged 13 commits into from
Jul 25, 2019
Merged

Add option to force forward slashes in paths #3334

merged 13 commits into from
Jul 25, 2019

Conversation

MartyLake
Copy link

closes #3331

@MartyLake
Copy link
Author

Hope the CI gets green =) FYI @sampsyo

sampsyo
sampsyo requested changes Jul 23, 2019
View reviewed changes
Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Looks great so far! Any chance you could add the new option to the documentation for both plugins? (See the .rst files in the docs directory.)

beetsplug/playlist.py Outdated Show resolved Hide resolved
beetsplug/playlist.py Outdated Show resolved Hide resolved
beetsplug/smartplaylist.py Outdated Show resolved Hide resolved
beets/util/__init__.py Outdated Show resolved Hide resolved
beetsplug/playlist.py Outdated Show resolved Hide resolved
beetsplug/playlist.py Outdated Show resolved Hide resolved
test/test_files.py Show resolved Hide resolved
beets/util/__init__.py Outdated Show resolved Hide resolved
@MartyLake
Copy link
Author

Thanks for the big review ! I have (if I am not mistaken) integrated all your suggestions and some doc. You can take another look @sampsyo =)

@MartyLake MartyLake force-pushed the martylake_add_forward_slash_option branch from 0080c24 to 076a82d Compare July 23, 2019 22:10
@sampsyo
Copy link
Member

sampsyo commented Jul 23, 2019

Cool! Could you take a look at the CI results? Looks like the new test is failing:
https://travis-ci.org/beetbox/beets/jobs/562817055#L915

And the style checker has some suggestions:
https://travis-ci.org/beetbox/beets/jobs/562817056#L1102-L1103

sampsyo
sampsyo reviewed Jul 23, 2019
View reviewed changes
docs/plugins/playlist.rst Outdated Show resolved Hide resolved
@MartyLake
Copy link
Author

MartyLake commented Jul 24, 2019 via email •
edited
Loading

@sampsyo
Copy link
Member

sampsyo commented Jul 24, 2019

That's fine, but because all paths in beets are bytestrings, it might be even more accurate to just change the test:

p = br'C:\a\b\c'
a = br'C:/a/b/c'

Review: simpler implementation and test
fb96660 
Because **all** the path are bytestrings
@MartyLake
Copy link
Author

Simplified it ! Tell me if you catch other things that went off my radar :)

@sampsyo sampsyo merged commit fb96660 into beetbox:master Jul 25, 2019
sampsyo added a commit that referenced this pull request Jul 25, 2019
@sampsyo
Merge pull request #3334 from MartyLake/martylake_add_forward_slash_o…
84032e5 
…ption

Add option to force forward slashes in paths
sampsyo added a commit that referenced this pull request Jul 25, 2019
@sampsyo
Spelling & changelog for #3334
9392256
@sampsyo
Copy link
Member

sampsyo commented Jul 25, 2019

Looks wonderful! Thank you for taking care of this! Merged. 😃 ✨ 🚀

@MartyLake
Copy link
Author

MartyLake commented Jul 25, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sampsyo sampsyo sampsyo requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

playlist: Add option to force forward slashes in paths
2 participants
@MartyLake @sampsyo