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

Support pathlib objects #7

Closed
wants to merge 3 commits into from
Closed

Support pathlib objects #7

wants to merge 3 commits into from

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Apr 16, 2019

Fixes sccn/xdf#41.

I've also managed to shave off one level of precious indentation space in case you're wondering why the change is so big.

@cboulay
Copy link
Contributor

cboulay commented Apr 18, 2019

The original with <open> as f: is preferred because it automatically closes the file when the context is left, either naturally at the end of loading or if there's an exception. In the PR form there is no f.close() so the file remains open.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 18, 2019

Of course I forgot the close 😄. I normally prefer the context manager too, but in this case I think it is better to explicitly call open and close because we really need that horizontal space.

@tstenner
Copy link
Contributor

What if an exception occurs somewhere before the close?

I'm all for reducing the indentation level, but it's not related to the pathlib changes and it interferes with a lot of other PRs.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 18, 2019

OK, I'll revert this, but we need to take care of exceptions anyway, because right now we catch all exceptions at the end. Technically, we could also add f.close to a finally block, but we can discuss this in a later PR.

@tstenner
Copy link
Contributor

Looks good to me. If @cboulay doesn't have any objections we should squash and merge it.

@cboulay
Copy link
Contributor

cboulay commented Apr 18, 2019

I merged a squashed version of this.

@cboulay cboulay closed this Apr 18, 2019
@cbrnr cbrnr deleted the pathlib branch April 30, 2019 05:47
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.

pathlib not supported
3 participants