Closed jahooker closed 3 months ago
very nice! will review properly some time after Thursday (prepping/running a workshop)
maybe @jojoelfe has some time now?
:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 82.73%. Comparing base (
5b19406
) to head (4e311c5
). Report is 7 commits behind head on main.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I'll try to look at this tonight, but can't really promise too much.
@jahooker Thanks so much for the PR! Can you maybe write one or two lines what the motivation for this is? I do think it's a bit nicer that the constructor does not immediately write to disk, but when would one want to do this?
Hi! Thanks for taking the time to consider my changes!
Quite a few times now, I have found myself wanting — in an interactive Python session — to see what starfile.write
would generate, but I do not want the hassle of coming up with a name for the output file (e.g. test.star
or delete_me.star
). Also, I have found myself wanting in scripts to use starfile
simply to print text. If I want to put the output in a file, I might use shell redirection (e.g. myscript.py > foo.star
). And finally, maybe the file system is super slow (or broken altogether) — computing infrastructure can have bad days, after all. My changes allow for a separation of the concerns of text generation and file writing. You might do:
#!python3
import starfile
from starfile.writer import StarWriter
from elsewhere import foo
bar = starfile.read('bar.star')
writer = StarWriter(foo(bar), '')
print(''.join(writer.file_contents()))
Admittedly, the interface could be nicer. Perhaps StarWriter.file_contents
should return a string outright, not a generator. And perhaps it should be possible to instantiate a StarWriter
without providing a filename (even a nonsensical one like ''
). To that end, I might make some more changes.
@alisterburt I don't think I can trigger the tests on this PR. Can you do it?
@jojoelfe done, thanks!
@jojoelfe I've given you admin here
Hey @jahooker, I've just added a few tests and minor fixes.
The speed test for python3.9 keeps on failing, but on my machine the new code does not cause performance recession with py3.9 and py3.10... Maybe it just gets assigned to a runner with performance problems.
If you don't have anything to add (maybe some documentation would be nice, but can also be done later) this seems good to got to me.
Thanks again!
Johannes
Great, I merged for now and created an issue for the documentation.
I've made a few small modifications to
StarWriter
. Now, you can instantiate the class without writing to disk. To write, you need to explicitly callStarWriter.write
. You can also just see the contents of the file that aStarWriter
would write, withStarWriter.file_contents
.