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.

Sign up for GitHub

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

Jump to bottom

Feature #32: Available Halo in jupyter notebook #40

Merged
merged 7 commits into from
May 6, 2018
Merged

Feature #32: Available Halo in jupyter notebook #40

merged 7 commits into from
May 6, 2018

Conversation

winterjung
Copy link
Contributor

@winterjung winterjung commented Jan 5, 2018 •
edited
Loading

Description of new feature

Now, Halo in jupyter notebook is available by implementing IPython widget. If you want, test manually on .ipynb(require download).

capture-gif

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

Opinion

I tried to find the way that can test IPython widget in unittest. I searched the tqdm, but they skip tests related with tqdm_notebook. So in this PR, I exclude halo_notebook.py from coverage by modifying .coveragerc file. And linting test isn't still working on my machine. :(

I have no experience with Python packaging, so I'm not sure it's ok to add dependency about ipywidgets. Please tell me what to do.

Related Issues and Discussions

Discuss on #32

People to notify

@manrajgrover

@winterjung winterjung changed the title #32: Available Halo in jupyter notebook Feature #32: Available Halo in jupyter notebook Jan 11, 2018
@winterjung
Copy link
Contributor Author

@manrajgrover Isn't any update here?

@manrajgrover
Copy link
Owner

@jungwinter I had faced the similar issue in lint but I thought it was fixed. It was because of some compatibility issue between Python2.7 and Python3.6. Looks like I would need to do a little debugging on it. See if you can reproduce the error on Python2.7.

The initial code looks great. I just tested it locally and it seems to work. I still need a little more time for reviewing the code though. Thanks for working on this. 😄 💯

@manrajgrover
Copy link
Owner

Regarding ipywidgets, since it is a requirement of Halo_Notebook, we need to add it to requirements. We can skip tests for this, but we should surely test any utilities being added.

@winterjung
Copy link
Contributor Author

@manrajgrover Thanks for the comment. I'll try to add tests for HaloNotebook. Maybe It looks like to check self.output.outputs of HaloNotebook class. 🚀

@winterjung
Change widget initializing position
d72779a 
Signed-off-by: jungwinter <wintermy201@gmail.com>

Before, the initializing position was in the `stop_and_persist` function. So we could not initialize `Output` widget with `stop` function.
After Changing position to `start` function, we can expect more consistent behavior.
@winterjung
Add testcase about HaloNotebook
7c95ad6
@winterjung
Copy link
Contributor Author

@manrajgrover How about this way when testing HaloNotebook?

@manrajgrover
Copy link
Owner

@jungwinter Just to let you know, I'm reviewing this in parts and that currently, this looks great.

Regarding testing the code, is there any other way to get the output that is being rendered? Because currently, this only tests the output of the code and not what is being rendered.

@winterjung
Copy link
Contributor Author

@manrajgrover Thanks for reviewing! As I know, this is the only way to test the output of ipywidget to be rendered. The outputs variable of the Output class matches the rendered output shown in the jupyter cell. I think it's not much different from the current way to read and seek _stream. If there is any way to test rendered output, I'll update my PR!

@manrajgrover manrajgrover self-requested a review March 26, 2018 12:02
@winterjung
Copy link
Contributor Author

@manrajgrover Isn't any update here?

@manrajgrover
Copy link
Owner

@jungwinter This is on high priority for me and I'll definitely finish reviewing and merging ASAP. Really sorry for the delay. Thanks for pinging.

manrajgrover
manrajgrover approved these changes May 6, 2018
View reviewed changes
Copy link
Owner

@manrajgrover manrajgrover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jungwinter Thank you so much for working on this and for your patience! 💯 I'm merging this as is since I have plans to revamp the code (especially threading, unicode issues and exception handlings). If I find a better way to test notebooks, I'll update the test cases. Great work! 😄

@manrajgrover manrajgrover merged commit e743969 into manrajgrover:master May 6, 2018
@manrajgrover manrajgrover mentioned this pull request May 6, 2018
Closed
@winterjung winterjung deleted the feature-notebook branch May 7, 2018 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manrajgrover manrajgrover manrajgrover approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@winterjung @manrajgrover