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

Use TTY::Cursor.hide instead of constants #39

Merged
merged 4 commits into from Dec 4, 2019
Merged

Use TTY::Cursor.hide instead of constants #39

merged 4 commits into from Dec 4, 2019

Conversation

benklop
Copy link
Contributor

@benklop benklop commented Nov 27, 2019

Describe the change

fixes issue #38

Why are we doing this?

crashing is not desirable

ECMA_CSI = "\x1b["
ECMA_CSI = "\x1b[".freeze
DEC_TCEM = '?25'.freeze
DEC_RST = 'l'.freeze

Choose a reason for hiding this comment

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

Style/RedundantFreeze: Do not freeze immutable objects, as freezing them has no effect.
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -17,7 +17,9 @@ class Spinner
# @raised when attempting to join dead thread
NotSpinningError = Class.new(StandardError)

ECMA_CSI = "\x1b["
ECMA_CSI = "\x1b[".freeze
DEC_TCEM = '?25'.freeze

Choose a reason for hiding this comment

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

Style/RedundantFreeze: Do not freeze immutable objects, as freezing them has no effect.
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -17,7 +17,9 @@ class Spinner
# @raised when attempting to join dead thread
NotSpinningError = Class.new(StandardError)

ECMA_CSI = "\x1b["
ECMA_CSI = "\x1b[".freeze

Choose a reason for hiding this comment

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

Style/RedundantFreeze: Do not freeze immutable objects, as freezing them has no effect.

@coveralls
Copy link

coveralls commented Nov 27, 2019

Coverage Status

Coverage increased (+0.02%) to 97.923% when pulling 2eb0c7a on benklop:master into 8765128 on piotrmurach:master.

@piotrmurach
Copy link
Owner

Hi,

Thanks for contributing!

These constants are remnants from my refactorings. In this case, they should be replaced by tty-cursor, you can see it here.

@piotrmurach
Copy link
Owner

Hey! Thanks for updating your PR. Would it possible for yourself to add a test for this as well so I don't break things in the future? That would be a cherry on a cake!

@benklop
Copy link
Contributor Author

benklop commented Dec 2, 2019

I'll take a look at creating a test, but I have some other work to complete first

spec/unit/spin_spec.rb Outdated Show resolved Hide resolved
spec/unit/spin_spec.rb Outdated Show resolved Hide resolved
spec/unit/spin_spec.rb Show resolved Hide resolved
@benklop
Copy link
Contributor Author

benklop commented Dec 3, 2019

@piotrmurach i'm not really used to the style you're using for your specs, hopefully context blocks are acceptable

@benklop benklop changed the title add missing constants Use TTY::Cursor.hide instead of constants Dec 3, 2019
@piotrmurach piotrmurach merged commit 4f395ed into piotrmurach:master Dec 4, 2019
@piotrmurach
Copy link
Owner

Thanks for making this change and adding tests! 🙇

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

Successfully merging this pull request may close these issues.

None yet

4 participants