fixed suboptimal usage of ipywidgets.Output widget

manrajgrover / halo

💫 Beautiful spinners for terminal, IPython and Jupyter

MIT License

2.86k stars 148 forks source link

fixed suboptimal usage of ipywidgets.Output widget #143

Open norweeg opened 4 years ago

norweeg commented 4 years ago

Description of new feature, or changes

Fixes suboptimal usage of ipywidgets.Output in HaloNotebook. See Output's API documentation for more info. resolves #140 resolves #59

Checklist

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

Related Issues and Discussions

People to notify

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 431

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at ?%

Totals
Change from base Build 429:	0%
Covered Lines:
Relevant Lines:	0

💛 - Coveralls

lujobi commented 4 years ago

Hi @norweeg Can I help in order to fix the pipeline? I tried locally with two python versions (2.7 and 3.7) both of them seem to succeed. Do you have an idea what the problem could be?

norweeg commented 4 years ago

Hi @norweeg Can I help in order to fix the pipeline? I tried locally with two python versions (2.7 and 3.7) both of them seem to succeed. Do you have an idea what the problem could be?

I suspect that these tests are bad. The code was originally doing all the formatting of the output and manually writing it into the output buffer, which is extremely unnecessary and defeats the purpose of using the Output widget, which are specifically intended to capture any output printed or displayed with print or display without any manual formatting to get it to display in Jupyter's output system. The tests appear to be looking for the output that would have been manually formatted and written in the buffer and therefore are bad tests. Basically, the result is the same, but the tests aren't looking at results, they're looking at how it produced the results, and the way that was originally used just did not use Output widgets correctly at all.

mnicstruwig commented 3 years ago

Gently bumping this, since it hasn't seen activity in a while, and also happens to affects me. :)

norweeg commented 3 years ago

Gently bumping this, since it hasn't seen activity in a while, and also happens to affects me. :)

I will get back on this! I need to write tests, but a significant amount of the existing tests need to be reworked as they check values of internal attributes of the ipywidgets.Output class when the whole purpose of this PR is to get away from directly formatting and writing output, but rather allow the Output widget to perform that function for us

manrajgrover commented 2 weeks ago

@norweeg Thanks for the PR, and my sincere apologies for the delay. I do agree testing requires a revamp. Do you plan to work on this?