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

[MRG+1] Allow passing Path objects to joblib.{dump,load}. #316

Merged
merged 1 commit into from Mar 7, 2016

Conversation

@anntzer
Copy link
Contributor

anntzer commented Mar 6, 2016

See #315.

@@ -389,6 +393,8 @@ def dump(value, filename, compress=0, cache_size=100, protocol=None):
# By default, if compress is enabled, we want to be using 3 by
# default
compress = 3
if Path and isinstance(filename, Path):

This comment has been minimized.

Copy link
@GaelVaroquaux

GaelVaroquaux Mar 6, 2016

Member

Nitpick: could you write this: "if Path is not None and isinstance(filename, Path)"

@@ -440,6 +446,8 @@ def load(filename, mmap_mode=None):
object might not match the original pickled object. Note that if the
file was saved with compression, the arrays cannot be memmaped.
"""
if Path and isinstance(filename, Path):

This comment has been minimized.

Copy link
@GaelVaroquaux

GaelVaroquaux Mar 6, 2016

Member

Same nitpick.

@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Mar 6, 2016

Beside the 2 nitpicks, that are just cosmetics to make the intent of the code more explicit, this is 👍 from me. Thank you very much!

@anntzer anntzer force-pushed the anntzer:pathlib-dump-load branch from ed866bf to f6107c1 Mar 6, 2016
@anntzer

This comment has been minimized.

Copy link
Contributor Author

anntzer commented Mar 6, 2016

fixed

@GaelVaroquaux GaelVaroquaux changed the title Allow passing Path objects to joblib.{dump,load}. [MRG+1] Allow passing Path objects to joblib.{dump,load}. Mar 6, 2016
@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Mar 6, 2016

LGTM. + 1 for merge

Can we have another review from the core team?

@aabadie

This comment has been minimized.

Copy link
Contributor

aabadie commented Mar 6, 2016

Can we have another review from the core team?
Just had a look, it seems all good to me as well. Maybe a mention to this new feature could be added in CHANGES.rst.

@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Mar 6, 2016

@anntzer

This comment has been minimized.

Copy link
Contributor Author

anntzer commented Mar 7, 2016

Should I create a "0.9.5" entry?

@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Mar 7, 2016

@anntzer anntzer force-pushed the anntzer:pathlib-dump-load branch from f6107c1 to 2d41970 Mar 7, 2016
@anntzer

This comment has been minimized.

Copy link
Contributor Author

anntzer commented Mar 7, 2016

Done.

@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Mar 7, 2016

LGTM, merging thanks !

lesteve added a commit that referenced this pull request Mar 7, 2016
[MRG+1] Allow passing Path objects to joblib.{dump,load}.
@lesteve lesteve merged commit 81e1a71 into joblib:master Mar 7, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Mar 7, 2016

@anntzer FYI you can use "Fix #issue_number" in the PR description so that merging a PR closes automatically the associated issue.

@anntzer anntzer deleted the anntzer:pathlib-dump-load branch Jan 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.