Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed suboptimal usage of ipywidgets.Output widget #143

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

norweeg
Copy link

@norweeg norweeg commented Mar 7, 2020

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
Copy link

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 Coverage Status
Change from base Build 429: 0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@lujobi
Copy link

lujobi commented May 11, 2020

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
Copy link
Author

norweeg commented May 12, 2020

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.

@norweeg norweeg force-pushed the halonotebook-fix-persist branch from 294247f to ff685a2 Compare August 4, 2020 16:32
@norweeg norweeg force-pushed the halonotebook-fix-persist branch from ff685a2 to 58c0080 Compare November 24, 2020 20:02
@norweeg norweeg force-pushed the halonotebook-fix-persist branch from 58c0080 to 374b97f Compare November 24, 2020 20:56
@mnicstruwig
Copy link

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

@norweeg
Copy link
Author

norweeg commented Jun 24, 2021

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
Copy link
Owner

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants