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 use of str(pathlib.Path(...)) in codebase #592

Closed
freakboy3742 opened this issue Jun 16, 2021 · 9 comments
Closed

Replace use of str(pathlib.Path(...)) in codebase #592

freakboy3742 opened this issue Jun 16, 2021 · 9 comments
Labels
bug A crash or error in behavior. good first issue Is this your first time contributing? This could be a good place to start!

Comments

@freakboy3742
Copy link
Member

Describe the bug

Brett Cannon tweeted this recently:

Python tip: anytime you accept a path that could be a path-like object (e.g. pathlib), never rely on its string repr; always use os.fsdecode(), os.fsencode(), or os.fspath() depending on what you want; None is probably not an acceptable path for your needs 😉. https://twitter.com/brettsky/status/1404521184008413184

This is something we do extensively in the Briefcase codebase... but I guess we shouldn't.

To Reproduce

Not sure - seeking clarification from Brett.

@freakboy3742 freakboy3742 added bug A crash or error in behavior. good first issue Is this your first time contributing? This could be a good place to start! up-for-grabs labels Jun 16, 2021
@papalotis
Copy link

I can try working on it

@freakboy3742
Copy link
Member Author

After further conversation with Brett, we might not be as badly affected as I initially thought.

I originally thought this would manifest as filesystem encoding problems; but it turns out it's "objects that have an str() representation but aren't paths" that are the problem - most notably, None.

We don't (or shouldn't) have any places where a None can turn up as a path - so this won't affect us too badly.

However, there are a lot of test cases where we included explicit calls to str() for compatibility with Python 3.5; and there will be a couple of places in the codebase where we're calling str() but should be calling os.fsdecode().

@papalotis You're welcome to take a look at this is you want! The task will be to audit everywhere in the codebase that we're currently explicitly invoking str(); If the argument is a pathlib object, update the code. This will involve either:

  1. Removing the str() call entirely (because it was introduced for Python 3.5 compatibility)
  2. Replacing the use of str() with os.fsdecode().

The slightly harder audit will be to look for all implicit invocations of str(), caused when a pathlib object is being passed to an f-string/str.format() call. It might make sense to do this as part of converting all the str.format() calls with their equivalent f-strings.

@iamzili
Copy link
Contributor

iamzili commented Jun 23, 2021

@papalotis Hi, are you working on this issue? If not I would like to take it.

@freakboy3742
Copy link
Member Author

@zili55 Based on the lack of response from @papalotis, I'd say the ticket is yours! If you've got any questions, ask away, either here or in Discord.

@mullera3
Copy link

mullera3 commented Jul 3, 2021

I would like to try working on it @freakboy3742

@freakboy3742
Copy link
Member Author

@mullera3 @zili55 is already working on this issue; they have been communicating with me on details outside this ticket. If you're interested in contributing to a different ticket, check out the first timers only tickets in our repo for some ideas.

@mullera3
Copy link

mullera3 commented Jul 3, 2021

Okay thank you! @freakboy3742

@iamzili
Copy link
Contributor

iamzili commented Sep 25, 2021

@freakboy3742 this issue can be closed, right?

@freakboy3742
Copy link
Member Author

Yes - this was corrected by #599.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A crash or error in behavior. good first issue Is this your first time contributing? This could be a good place to start!
Projects
None yet
Development

No branches or pull requests

4 participants