StarWriter.lines

[jahooker]

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 call StarWriter.write.
You can also just see the contents of the file that a StarWriter would write, with StarWriter.file_contents.

[alisterburt]

very nice! will review properly some time after Thursday (prepping/running a workshop)

[alisterburt]

maybe @jojoelfe has some time now?

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.73%. Comparing base (5b19406) to head (4e311c5).
Report is 7 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   80.93%   82.73%   +1.80%     
==========================================
  Files           7        7              
  Lines         278      307      +29     
==========================================
+ Hits          225      254      +29     
  Misses         53       53              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jojoelfe]

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?

[jahooker]

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.

[jojoelfe]

I do think this usecase makes sense. There are a couple thoughts I have:

• I guess there is a chance that this breaks some existing if somebody used the StarWriter class. But as far as I can tell the StarWriter class is not documented anywhere, so I don't think this is a large concern.
• Since the StarWriter class is not really supposed to be a public API, wouldn't it be nice to have an exposed function so one could do this:

import starfile
from elsewhere import foo

bar = starfile.read('bar.star')
starstring = starfile.to_string(foo(bar))

• I do like having a generator for the lines, as this might help people who want to stream/paginate starfiles. But to me looks like in the current state the actual data is returned as a single string by the file_contents() function.
• If we do not care about the API stability, it would make sense to remove the filename from the constructor and instead make it an argument to the write() method

[jojoelfe]

@alisterburt I don't think I can trigger the tests on this PR. Can you do it?

[alisterburt]

@jojoelfe done, thanks!

@jojoelfe I've given you admin here

[jojoelfe]

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

[jahooker]

Thanks @jojoelfe! Looks good. I'll write a little bit of documentation.

[jojoelfe]

Great, I merged for now and created an issue for the documentation.