-
-
Notifications
You must be signed in to change notification settings - Fork 728
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
refactor: add type hints #774
refactor: add type hints #774
Conversation
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.
Reviewed to line 2000
@@ -220,19 +223,19 @@ def highlight_text(text): | |||
return "".join(ansiSplit) | |||
|
|||
|
|||
def gef_print(x="", *args, **kwargs): | |||
def gef_print(x: str = "", *args: Tuple, **kwargs: Dict) -> Optional[int]: | |||
"""Wrapper around print(), using string buffering feature.""" | |||
x = highlight_text(x) | |||
if gef.ui.stream_buffer and not is_debug(): | |||
return gef.ui.stream_buffer.write(x + kwargs.get("end", "\n")) | |||
return print(x, *args, **kwargs) |
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.
weird. Why do we return print here? Mind changing to print then return? This whole function can probably just return None, actually.
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 actually encountered quite some similar questionable practices along the way ;)
In regards to changing this code in this PR: If I were to start changing things so it makes more sense this PR will become even huger and very confusing. IMO code changes should be implemented in separate PRs.
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.
which is fine, but we will forget unless we note this done somewhere. Maybe make another issue and just add notes from this PR for things to fix?
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 opened a project for this at https:/hugsy/gef/projects/11
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.
Reviewed lines 2000-4000
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.
Reviewed lines 4000-10000
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.
Reviewed to end of file
Looks good to me! Since this is targeting @hugsy 's branch let's make sure he's OK merging into it. |
refactor: add type hints
Description/Motivation/Screenshots
This huge PR adds type hints to the gdb8/py36 refactor branch and thereby reveals some logical bugs that can be addressed moving forward.
(Probably this PR does not cover 100% of the places where python 3.6. typing hints could be added but I wanted to push it already because I'm not sure about my schedule these next days)
How Has This Been Tested?
make test
make lint
Checklist
gdb_8_py36_code_refactor
branch, notmaster
.EDIT:
The PR was tested with python version 3.6 to be sure of compatibility
EDIT:
Just FYI: The tests that fail on my local machine are
test_cmd_elf_info
andtest_cmd_unicorn_emulate
. And I also set up a CI with docker and ubuntu 18.04 and 20.04 where onlytest_cmd_set_permission
fails (but not the above mentioned). still haven't figured out why that is,