-
Notifications
You must be signed in to change notification settings - Fork 148
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
{Issue #5}: Whitelist windows spinners #122
base: master
Are you sure you want to change the base?
{Issue #5}: Whitelist windows spinners #122
Conversation
Pull Request Test Coverage Report for Build 362
💛 - Coveralls |
Yikes, I checked AppVeyor and it seems that Edit: Found out why. I'm getting this warning when I try to install
|
By luck, coincidence, or design (I'll take either), the last version of pylint for Python 2.7 passes all tests:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoseALermaIII Thanks for looking into the issues and opening this PR! 😄
I've left a few comments which would need attention before this can be merged.
|
||
instance.spinner = "monkey" | ||
self.assertEqual(default_spinner, instance.spinner) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add test where we render the frames in windows environment and check if they are working correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I saw a similar test higher up. I'll check if it can be implemented in Windows
@manrajgrover Went with
|
Description of new feature, or changes
Whitelists spinners that are compatible with Windows OS in
halo.py
's_get_spinner()
method instead of always defaulting toline
. Still defaults toline
if spinner is not in the whitelist.Checklist
I don't have much experience with testing, so I have to ask: is it being tested properly? Double-checking the
tox
output forhalo.py
, line 270 is theis_supported()
check and lines 271-274 are only used in a non-Windows environment.Related Issues and Discussions
Partially fixes #5 except for
This could be part of #6, which I can address using Sphinx in another PR.
It also doesn't address
I don't know what Python 2.7.x will have issues with, but I can try it out.
People to notify
@manrajgrover