search

plottertools / vpype-gcode

gcode extension for vpype
MIT License
36 stars 7 forks source link

Better handle the output file #10

Closed theomega closed 3 years ago

theomega commented 3 years ago

Using click.File we can be sure that the file is opened. Additionally, the actual opening and closing of the file can be left to click and one line of indent can be removed. Thus the code is simlified.

Also as a nice bonus, this allows to output the gcode to stdout by providing "-" as output file on the command line similar to typical unix tools.

theomega commented 3 years ago

It looks like a bigger change than it is. Actually it is just removing the open call and its indent and replacing all f with output. This is a non breaking change, all operations which worked previously should also work after this is merged.

theomega commented 3 years ago

I think we have to decide: Do we want to be able to include the filename in the output, then we can't use click.File as the output could also be stdout or other non-files. This PR should be closed then and not merged.

On the other side, this would simplify the code base.I leave the call to @tatarize.

tatarize commented 3 years ago

I'm done editing the code probably for a good long while... and likely stepped on your toes for this one a bunch of times. Should be good now.

tatarize commented 3 years ago

Yes, in principle, that's the better way to handle the output file and if done that way I'd entirely take merge that. Specifically for the reason that that is what write does within vpype.

@click.argument("output", type=click.File("w")) is directly from the vpype code. Making them generally the same or more similar is a fine way of doing that.

On the flip side there's always a weirdness with opening files with cli parsers because they don't actually get closed at the end very well, and sometimes get stuck open for the duration of the program. It's not much of a trouble here since it's quitting the program right afterwards.

abey79 commented 3 years ago

This is the right thing to do @theomega 👍🏻

Do we want to be able to include the filename in the output, then we can't use click.File as the output could also be stdout or other non-files.

1) Click has facilities to recover the name: output.name. I'm using this in write's implementation. 2) I think its fine to expose file name to the .format(). Users will just have to cope with it shows up as <stdout> when they use -.

abey79 commented 3 years ago

Side Q: is it normal that both dx and _dx params to format() are passed the save value?

tatarize commented 3 years ago

Side Q: is it normal that both dx and _dx params to format() are passed the save value?

Nah, just a bug. Corrected in 0.6.2

tatarize commented 3 years ago

Okay, and I moved and checked these changes in 0.6.5 for #10, ported them in and they seem to work perfectly fine. Except that the stream doesn't flush and close() like it does with a with so I had to add that to the end.

tatarize commented 3 years ago

I'm pretty sure this was in draft because of the lack of close() unless there's some other issue within this PR I missed I'll go ahead and close it in a day or two.