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

update example in README #537

Open
vpoulailleau opened this issue May 3, 2022 · 6 comments
Open

update example in README #537

vpoulailleau opened this issue May 3, 2022 · 6 comments
Labels
docs Issues related to documentation

Comments

@vpoulailleau
Copy link

Currently, the example count_python_loc is misleading, the normal Python version is way easier than what is suggested, at least if you don't consider Python versions that are end of life (https://devguide.python.org/#status-of-python-branches and https://devguide.python.org/devcycle/#end-of-life-branches).

The normal version should be something like:

from pathlib import Path

def count_python_loc(path):
    """Count non-blank lines of Python code."""
    return sum(
         sum(1 for line in file.read_text() if line.strip())
         for file in Path(path).rglob("*.py"))

To my mind, this should be reflected in the documentation. And maybe a read_text and a read_bytes method equivalent could be added. What's your opinion on this?

@lurch
Copy link
Contributor

lurch commented May 6, 2022

I agree that it might make sense to use "more modern" Python in the example, but that probably can't happen until after #317 has been resolved?

@vpoulailleau
Copy link
Author

@lurch Thanks for the info.

I agree with the last comment in the mentioned issue (#317 (comment)), and yes, to my mind, #317 should be closed first (or simultaneously with this issue).

@althonos
Copy link
Member

althonos commented May 6, 2022

I suppose we could have both, pathlib in available in all Python3 versions nowadays, but I still find myself using os.path out of habit so I'd rather still have the current example as well.

@althonos althonos added the docs Issues related to documentation label May 6, 2022
@vpoulailleau
Copy link
Author

I quite disagree, according to PEP 20: "There should be one-- and preferably only one --obvious way to do it."

We should choose between os.path and pathlib.

According to https://docs.python.org/3/library/pathlib.html ("For low-level path manipulation on strings, you can also use the os.path module.") I would vote for pathlib as higher level lib (os.path may be needed in specific cases for small performance gains)

But I let the final decision to the project contributors.

@vpoulailleau
Copy link
Author

@lurch
Copy link
Contributor

lurch commented May 7, 2022

I've just given this a bit more thought... the pyfilesystem API is broadly modelled around the old os.path API (as that was all that existed when pyfilesystem was first created); so perhaps it doesn't really make sense to switch the non-pyfilesystem-example to use pathlib, until / unless pyfilesystem also has a more pathlib-esque API? 🤷 (see #238 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues related to documentation
Projects
None yet
Development

No branches or pull requests

3 participants