Fixes #56: Add placement parameter that allows spinner on right side of text #57
Conversation
- Defaults to `left` - Allows both `left` and `right` - When set to `right` will display spinner on right side of text
- now immutable
There was a problem hiding this comment.
@stephen-bunn I've added few comments which might need attention. Rest looks good to me. Could you please check why tests are failing on Appveyor?
| @@ -63,6 +67,7 @@ def __init__(self, text='', color='cyan', spinner=None, animation=None, interval | |||
| if not stream: | |||
| stream = sys.stdout | |||
|
|
|||
| self.placement = placement | |||
There was a problem hiding this comment.
self.placement -> self._placement
| Defines the placement of the spinner | ||
| """ | ||
| if placement not in self.SPINNER_PLACEMENTS: | ||
| raise ValueError("unknown spinner placement '{0}', available are {1}".format(placement, self.SPINNER_PLACEMENTS)) |
There was a problem hiding this comment.
Do you like the idea of raising ValueError on unknown placements?
The reason line 70 is self.placement = placement (not self._placement = placement) is because it calls this setter method which runs the snippet above.
I can simply remove the logic in the setter and say any placement that isn't right is assumed to be left.
There was a problem hiding this comment.
No, it makes sense to throw an error. This is correct.
| """Test right placement of spinner. | ||
| """ | ||
| spinner = Halo(text='foo', placement='right', stream=self._stream) | ||
| spinner.start() |
There was a problem hiding this comment.
I would suggest you add a time.sleep between .start and .succeed which will not stop the spinner immediately and there would be output that you can assert on. Perhaps this is the reason why test is failing on Appveyor. Get the output after time.sleep and succeeding.
tests/test_halo_notebook.py
Outdated
| output = self._get_test_output(spinner) | ||
|
|
||
| (text, _) = output[-1].split(" ") | ||
| self.assertEqual(text, "foo") |
There was a problem hiding this comment.
Likewise here. Could you please debug this?
- adding time.sleep after `spinner.start` calls in tests
There was a problem hiding this comment.
LGTM! Thank you for working on this @stephen-bunn and for your patience. 💯
Description of new feature, or changes
Adds the
placementparameter which allows placement of the spinner on either the right or left side of the text using the valuesrightandleft.Defaults to
leftThe only real logic changes are checking if the placement value is set to
rightand then switching the text and spinner frames in astr.fomatcall.Opening additional PR (previous #52) because original repo was removed.
Checklist
Related Issues and Discussions
Fixes: #56
Closes: #52
People to notify
@manrajgrover