Fixes #97: prevent writes to closed streams

[theY4Kman]

Description

This PR introduces a new settable property, Halo.enabled, and extends the definition of "enabled" to also include whether or not the stream is closed — i.e. even if Halo(stream=ganges_file, enabled=True) is passed, Halo.enabled will be False if ganges_file.closed. All places referencing the old Halo._enabled attr have been updated to use the property (like stop and clear).

Additionally, _render_frame in the spinner thread has been augmented to skip writing the frame text if not Halo.enabled.

The upshot of these changes is: running Halo under pytest is all kosher.

Questions

1. Even when Halo.enabled is False, I kept _render_frame calling Halo.frame() to generate the frame text and, more importantly, incrementing the frame counter, so the proper frame in time would display if the stream was reopened — I wasn't sure whether it made more sense to keep the animation consistent with the time passed, or totally pause so as to keep the animation consistent with the currently displayed frame. What do you think?

2. I kinda just baked the concept of the stream being closed into Halo.enabled. I wonder if it makes more sense to keep the two concepts separate, each checked explicitly. Like, are people gonna be surprised with Halo if they execute spinner.enabled = True; print(spinner.enabled) and get False (because their stream was closed)? Will they think to check their stream? Maybe a separate Halo.can_display prop/method, to keep Halo.enabled pure and with one responsibility?

3. I used a settable property for Halo.enabled. Uh, I guess I just wanted to point it out, 'cause I wasn't sure why it wasn't a public property, so maybe I missed a deliberate decision to hide it.

Checklist

• [X] Your branch is up-to-date with the base branch
• [X] You've included at least one test if this is a new feature
• [X] All tests are passing

Related Issues and Discussions

Resolves #97

People to notify

@tterb

[coveralls]

Pull Request Test Coverage Report for Build 391

• 39 of 41 (95.12%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.8%) to 91.019%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
halo/halo.py	34	36	94.44%
<!--	Total:	39	41	95.12%	-->

Totals
Change from base Build 390:	0.8%
Covered Lines:	306
Relevant Lines:	330

---

💛 - Coveralls

[manrajgrover]

Another great PR @theY4Kman! :100: Thanks a bunch!

The reason enabled wasn't exposed as a public property was because it is not complete (if you see here you will see my comment). Can we check if the stream is closed in setter instead of getter?

Also, has this been tested in bug conditions?

[theY4Kman]

Cheers!

I put the check in the getter, because the stream could close any time, not just when setting the value. What do you think about adding a check for stream.closed in the setter, and raising an exception to fail early for the user?

I downloaded the repl.it pytest example and confirmed it failed without the change, then succeeded with the change. But I'd love to see @tterb confirm that in his env.

[tterb]

@theY4Kman Looking at the changes it looks like this should be a fix, but I'll test it in my env when I get home this evening to be sure.

Edit: Just checked and the changes do fix the pytest error in my env. Thanks!

[manrajgrover]

@theY4Kman I was thinking where should closing of stream be checked. Since enabled is a property set by user, we shouldn't override it based on stream since expected value shouldn't depend on stream. Can we move the logic out of enabled property and make an internal check whether we should write in stream or not? I think this would make more sense.

[theY4Kman]

@theY4Kman I was thinking where should closing of stream be checked. Since enabled is a property set by user, we shouldn't override it based on stream since expected value shouldn't depend on stream. Can we move the logic out of enabled property and make an internal check whether we should write in stream or not? I think this would make more sense.

Word; as I was writing the question in the PR, a separate check seemed to make more sense. I'mm done do it :)

[theY4Kman]

Oh, if the stream is not writable, but Halo.enabled is True, do we want to keep incrementing the frame counter? I'm gonna roll with "yes" for now — holler if ya think otherwise.

Edit: hmmm, I suppose it's pretty rare to have a stream obj reopen after closing.

[manrajgrover]

@theY4Kman Yup, it makes sense to generate frame but not render it if enabled is set to True and stream is closed. This seems like a rare and an edge case but I think this should be the correct behaviour.

[manrajgrover]

@theY4Kman There are some merge conflicts that need to be resolved before I can review and merge this. Let me know if you need any help! I would love to get this one merged.

[cSchubes]

is there any plan to actually merge this and fix the problem? This makes halo utterly unusable with pytest.

[theY4Kman]

merge conflicts resolved. she's ready to go, @manrajgrover :) apologies for the long delay

[manrajgrover]

@cSchubes This should be merged soon! Apologies for the delays.

[cSchubes]

@manrajgrover thanks so much! looking forward to it

[manrajgrover]

@theY4Kman Heyo! 😄 Apologies for pinging but I wish to merge this as soon as possible. Please let me know if you need any help or are facing any issues.

[theY4Kman]

I've shored up the tests. Holler if ya spot anything else.

[cSchubes]

how are we looking on this? would love to get a new release out soon