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

File APIs #25

Closed
ExpHP opened this issue Mar 1, 2016 · 8 comments
Closed

File APIs #25

ExpHP opened this issue Mar 1, 2016 · 8 comments

Comments

@ExpHP
Copy link
Contributor

ExpHP commented Mar 1, 2016

Feature request:

Currently f90nml only accepts filenames for its input and output files. This generally restricts the usage of f90nml in comparison to other serialization libraries in Python, which usually have methods for working with arbitrary file-like objects and string data (E.g. json and pickle have dump/load for file-like objects and dumps/loads for strings. xml.etree has parse which accepts both filenames and file-like objects, and fromstring for strings.)

For some examples of how these APIs are useful: Currently, it is not possible to parse an NML file directly from sys.stdin or to write one to sys.stdout, because these are already-open files with no path. It is similarly difficult to apply multiple patches in a row (whereas with a string api it would just be for patch in patches: s = f90nml.patchstr(s, patch)). Currently I work around these limitations using NamedTemporaryFile, which has a fair number of quirks about it and is something I generally consider to be a last resort.

Some thoughts:

  • If it's too much to add both, either one will do. It's easy for client code to write file-based wrappers around string functions (by calling f.read(), f.write()), or to write string-based wrappers around file functions (using StringIO).
  • because there is already patch which takes two files, I feel that the API would be cleanest if read, write, and patch were modified to detect if their arguments are paths or a file-like object (e.g. with hasattr(infile, 'read')). This way a user could mix and match, e.g. read from a file object and write to a path. In any case, though, strings will require their own functions (e.g. readstr(s), writestr(nml), and patchstr(s, patch) or however you want to name them).
  • I suppose there is also the question of whether patchstr should return just a string, or if it should return (string, nml) (since patch returns an NML).
  • One slight hurdle is that, unlike a path-based API, a file-based API should not call close. I bring this up because there are several calls to close() currently embedded inside the parsing functions.
@marshallward
Copy link
Owner

Yes, using paths is a bit dumdum, a bad decision that I now feel a bit stuck with. I've mostly been waiting for someone to object before changing this. You win the prize!

I'm willing to break the API (techincally, it's not a 1.0 yet) but I think the approach of xml.etree is the way to go here.

As for all the close statements around the exception raising, it's a problem but it should be solvable (at worst with some sort of opened flag). One of the reasons I regretting not using file objects.

I'll get started on this.

@ExpHP
Copy link
Contributor Author

ExpHP commented Mar 1, 2016

Ah, so fast! I began to take a stab at it myself shortly after posting (using a slightly different approach which looked something like this, which is crap by the way and I've already found a couple bugs), but certainly you know the code better, so I'm glad you're already on top of this! :D

@marshallward
Copy link
Owner

That patch looks OK to me, do you want to send it as a pull request? I can modify it if there's any problems with it.

@marshallward
Copy link
Owner

I just gave this patch a try, I think the finally may be closing files prematurely. Possibly because I rely on exceptions to handle a lot of internal operations. Just speculation though, I haven't confirmed it yet.

I'd be interested to hear how you go with it.

edit Sorry, false alarm, I didn't have the return statement inside the try block. It looks OK to me.

@ExpHP
Copy link
Contributor Author

ExpHP commented Mar 1, 2016

I see some of the ideas have already made it in there. I could submit a
pull request, though I feel I should take a moment to draft up some test
cases.

P.S. in that link there were actually two patches, where the second one is
a second version I made after fixing some of the bugs I found. It has a
nested try..finally which looks kind of ugly, but it prevents calling
nml_file.close() if open(nml_fname) fails.

On Tue, Mar 1, 2016 at 5:53 AM, Marshall Ward notifications@github.com
wrote:

I just gave this patch a try, I think the finally may not closing files
prematurely. Possibly because I rely on exceptions to handle a lot of
internal operations. Just speculation though, I haven't confirmed it yet.

I'd be interested to hear how you go with it.


Reply to this email directly or view it on GitHub
#25 (comment).

This was referenced Mar 1, 2016
@ExpHP
Copy link
Contributor Author

ExpHP commented Mar 23, 2016

I'll close this now that v0.18 has file APIs

@ExpHP ExpHP closed this as completed Mar 23, 2016
@marshallward
Copy link
Owner

Did you still want to add string support (reads)? If so, that can go under a separate issue.

@ExpHP
Copy link
Contributor Author

ExpHP commented Mar 23, 2016

Yeah, I still think they're a good idea for ergonomics. I didn't push too hard for them since unlike the file apis, these can be implemented as wrapper functions.

I will open a separate issue

@ExpHP ExpHP changed the title File or string APIs File APIs Mar 23, 2016
@ExpHP ExpHP mentioned this issue Mar 23, 2016
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

No branches or pull requests

2 participants