-
Notifications
You must be signed in to change notification settings - Fork 54
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
Cyber #420
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #420 +/- ##
==========================================
- Coverage 74.79% 73.91% -0.88%
==========================================
Files 269 271 +2
Lines 22085 22460 +375
==========================================
+ Hits 16518 16601 +83
- Misses 5567 5859 +292
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
074a99b
to
b8bf519
Compare
max_rows, max_cols = _get_win_size() | ||
|
||
orig_row, _ = _get_cursor_pos() | ||
|
||
_cursor_hide() | ||
_cursor_move(orig_row, orig_col) | ||
|
||
characters, remaining, (orig_row, orig_col), _ = _get_character_info( | ||
buf, max_rows, max_cols, orig_row, orig_col | ||
) |
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.
This and the data on lines 222:232 are pretty similar, maybe an idea to put it in a _setup()
? or something like that.
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.
or, put the setup and cleanup inside a context manager (maybe inside _get_terminal()
) and let it return the information from _get_character_info
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.
There's a combination of setting stuff up and retrieving some info that's used slightly differently. I'm fine with this small amount of repetition because it's also pretty concise.
if os.getenv("CYBER") == "💊": | ||
matrix(buf, color, mask_space) | ||
else: | ||
nms(buf, color, mask_space) |
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.
An idea to put the with _set_terminal()
context manager here? as both these functions use it in the same way.
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'd like the nms
and matrix
functions to be individually usable, so keeping the context manager in there is the best way.
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.
LGTM
Cyber