Bug: When used as a context manager, spinner starts itself

norweeg commented 5 years ago

Description

When used as a context manager, a Halo spinner starts itself before .start() is called.

Expected behaviour

The spinner should not start until .start() is called explicitly

Steps to recreate

with Halo(color="grey",spinner="clock",placement="left", animation="marquee") as spinner:
    try:
        with db.authenticate_user as db_connection: #prompts for username/password, hidden if spinner is running
            spinner.start("doing something")
            #do something
    except DBException:
        spinner.fail("caught exception")

tdsmith commented 5 years ago

The examples in the README seem to suggest that self-starting is the expected behavior; will that change?

norweeg commented 5 years ago

@tdsmith it auto starts even if the Halo object is returned by its stop method. Something is definitely wrong with it. Also, if you want it to start when used as a context manager, you can by calling the start method which returns the Halo object

manrajgrover commented 5 years ago

Hi @norweeg and @tdsmith,

Thanks for open the issue and apologies for such a late reply.

The PR in question would be https://github.com/manrajgrover/halo/pull/9.

Currently, things seem to be working as intended as per the feature request. I do agree with your example as well and that it should not auto start on object creation. Introducing this fix would be a breaking one and that should be introduced in 0.1.0 which I plan to release soon.

The examples in the README seem to suggest that self-starting is the expected behavior; will that change?

@tdsmith Do you also have a usecase for fixing this? Please share an example.

it auto starts even if the Halo object is returned by its stop method.

@norweeg I didn't quite get this. Need more information here.

tdsmith commented 5 years ago

Do you also have a usecase for fixing this?

No, the existing behavior seems like the right behavior to me, so I was surprised that you considered it to be a bug. I think most users will expect the context manager to start a spinner immediately; requiring users to explicitly start the spinner will just add verbosity, making the context manager less elegant.

The context manager is just a shortcut for a common case, so perhaps it is not a good tool for @norweeg's application, and he should avoid it. An option to disable auto-starting could also be useful, though all options add maintenance cost.

norweeg commented 5 years ago

@manrajgrover I meant that creating the spinner with with Halo(color="grey",spinner="clock",placement="left", animation="marquee").stop() as spinner: doesn't create the Halo object in a stopped state, but then I realized that you're probably starting it in the __enter__ method, so creating it in a stopped state does nothing. I guess ultimately I'm frustrated by the fact that I love the spinner, but it masks any kind of console IO. I was using it in a threaded application and kept thinking my code was deadlocking before I realized that my code was awaiting input I never saw because the the prompt because Halo clobbered it. I think I actually agree with @tdsmith now, but I would love it if Halo could listen for things printed to stdout and stderr and maybe call info() and warn() on them before resuming normal spinning with the the Halo object's original text, respectively, so they don't get masked when using a Halo spinner. It would also be great if there could be some way I could temporarily suspend a spinner so I can do console IO. Maybe on each pass of the spinner's loop it could acquire a Lock object that I could also acquire in my code whenever I wish to do some console IO and release it, allowing the Halo spinner to resume exactly where it was before my thread acquired its lock.

manrajgrover commented 5 years ago

@norweeg Apologies for the troubles caused. I expect users to pick the best way to use these spinners. I think the given option of controlling spinners without context manager should work for your case.

I can totally understand why it is desirable to stop spinner during IO but this is something I expect the user to do it manually and is possible even in the case of context manager.

I will surely investigate how similar libraries in other ecosystem work in such scenarios and see if it is actually okay to take away control from user during IOs.

It would also be great if there could be some way I could temporarily suspend a spinner so I can do console IO. Maybe on each pass of the spinner's loop it could acquire a Lock object that I could also acquire in my code whenever I wish to do some console IO and release it, allowing the Halo spinner to resume exactly where it was before my thread acquired its lock.

Did you try stopping the spinner and starting it again after IO? Did it work as expected?

Thanks for explaining the usecase! Also, apologies for long delay in getting back. I'm in the middle of switching roles these days.

norweeg commented 5 years ago

@manrajgrover yeah, its fine now. I did have one observation though:

if you're using HaloNotebook as a context manager in a Jupyter notebook, the output vanishes after you leave the context instead of persisting. The work around is to just not use it as a context manager.

I also have 2 questions: if you decorate a function with Halo decorator:

do you get a Halo object or a HaloNotebook object? Does it depend which you imported?
is there a way to reference that Halo object within the code to, for example, to call its info, warn, success, or fail methods, or does a decorated function just run then stop when the function returns?

norweeg commented 4 years ago

Didn't realize I left this open.

manrajgrover / halo