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

add options for several IO operation #362

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

add options for several IO operation #362

wants to merge 11 commits into from

Conversation

tzing
Copy link

@tzing tzing commented Dec 6, 2019

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

Pre discuss in #361, this PR add options for readbytes and readtext in each class.

Also it changes a behavior in FS.open, I found this when I was investigate if there's any other thing to be changed.
It seems no extra parameters are required for iotools.make_stream, and the description for options is arguments for the filesystem. So I guess it was misplaced and move options for openbin inside the function.

it seems no extra parameters are required for `iotools.make_stream`, and
the description for `options` is 'arguments for the filesystem'. so I
guess it was misplaced.
CHANGELOG.md Outdated Show resolved Hide resolved
@chfw
Copy link
Contributor

chfw commented Dec 6, 2019

Just an observation, readtext is as readable as read_text. But I guess it is down to historical design decision.

@lurch
Copy link
Contributor

lurch commented Dec 7, 2019

And maybe it's worth adding docstrings like this too?

the syntax error is cause by the trailing comma in func def, which is
added by black.

pyproject.toml is added for black to announce the target python version
related: psf/black#419
@tzing tzing changed the title add options for readbytes and readtext add options for several IO operation Dec 8, 2019
@lurch
Copy link
Contributor

lurch commented Dec 8, 2019

Question for @willmcgugan - looking at https://docs.pyfilesystem.org/en/latest/reference/base.html , should all functions that take path parameter(s) also take an options kwargs?

@tzing Sorry for potentially opening a can of worms... 😕 Your PR looks great so far IMHO 👍

@tzing
Copy link
Author

tzing commented Dec 14, 2019

ping @willmcgugan

@tzing tzing mentioned this pull request Mar 6, 2020
9 tasks
@althonos althonos added this to the v2.5.0 milestone Sep 19, 2020
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.

4 participants