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

Variadic gef print #789

Merged
merged 7 commits into from
Jan 21, 2022
Merged

Variadic gef print #789

merged 7 commits into from
Jan 21, 2022

Conversation

Grazfather
Copy link
Collaborator

@Grazfather Grazfather commented Jan 15, 2022

  • Make gef_print take variadic arguments like print does.
  • Make gef_print not return anything like print does
  • Make warn, info, etc. not return anything since gef_print doesn't.
  • Cleanup some formatting/prints.

Closes #788

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
@Grazfather Grazfather force-pushed the variadic_gef_print branch 2 times, most recently from 71775b1 to ad50f72 Compare January 15, 2022 23:12
@hugsy
Copy link
Owner

hugsy commented Jan 16, 2022

As mentioned briefly on Discord, wouldn't be simpler to just do this

        gef_print(f"\tParent PID {RIGHT_ARROW} {state['Pid']}"
                  f"\tCommand line {RIGHT_ARROW} '{cmdline}'")

@Grazfather
Copy link
Collaborator Author

As mentioned briefly on Discord, wouldn't be simpler to just do this

        gef_print(f"\tParent PID {RIGHT_ARROW} {state['Pid']}"
                  f"\tCommand line {RIGHT_ARROW} '{cmdline}'")

there wouldn't be newlines between each line in that case, they'd be concatenated to one line. Look at my comments above.

Base automatically changed from gdb_8_py36_code_refactor to dev January 16, 2022 22:55
@Grazfather
Copy link
Collaborator Author

Yes, this probably should have been a 'discussion' (I am not very familiar with them). We can close, but I want to keep the diff & have @theguy147 & @daniellimws weigh in oh what they think about this approach.

@daniellimws
Copy link
Collaborator

For me, slightly confusing at the start but should get used to it after a while. I think it could be worth the change if gef_print has more settings in kwargs (other than just end) but I can't think of any at the moment.

@theguy147
Copy link
Collaborator

theguy147 commented Jan 18, 2022

I'm for the "variadic" printing but I don't like how the end arg is used to join the strings when there is already a proper way to do so: the sep argument of python's builtin print.

How about something like this?:

def gef_print(*args: str, sep: str = "\n", **kwargs: Any) -> Optional[int]:
    """Wrapper around `print()`, using string buffering feature."""
    highlighted = list(map(highlight_text, args))
    if gef.ui.stream_buffer and not is_debug():
        return gef.ui.stream_buffer.write(sep.join(highlighted))
    print(*highlighted, sep=sep, **kwargs)

EDIT:
And if we want to do something really naughty we could also remove that (useless?) int return value:

def gef_print(*args: str, sep: str = "\n", **kwargs: Any) -> None:
    """Wrapper around `print()`, using string buffering feature."""
    highlighted = list(map(highlight_text, args))
    if gef.ui.stream_buffer and not is_debug():
        gef.ui.stream_buffer.write(sep.join(highlighted))
    else:
        print(*highlighted, sep=sep, **kwargs)

We never even use the int value except for err()/warn()/ok()/info() where we just bubble it up to the caller and then disregard it... (these 4 functions should also be changed to return None IMHO)

EDIT:
this is just an example snippet. of couse we could also use list comprehension instead of map in this scenario.

@Grazfather
Copy link
Collaborator Author

Grazfather commented Jan 19, 2022

I'm for the "variadic" printing but I don't like how the end arg is used to join the strings when there is already a proper way to do so: the sep argument of python's builtin print.

Yeah, I was using end because I saw separate args as basically short hand for separate invocations, which would add end to the end of each one. My PR actually drops the last 'end'. (A bug a just noticed).

Agreed on the rv of all those functions. They should be removed.

I think that we should support variadic using sep since that's basically just making it more print like.
But that means that most of the changes to use it variadically would need to set sep="\n" which uglies up a lot of the prettiness gained. We can instead change some of those cases to instead use the auto string concatenation with \n at the start of the line. That would basically make this PR useless (for now).

@theguy147
Copy link
Collaborator

theguy147 commented Jan 19, 2022

But that means that most of the changes to use it variadically would need to set sep="\n" [...]

That's why in my draft I set sep="\n", so the newline is the default value. This way only when it's supposed to be different it needs to be touched by the calling code.

@Grazfather
Copy link
Collaborator Author

Yeah, but I am not sure if we want gef_print to change sep so it doesn't match print's. That seems confusing.

@theguy147
Copy link
Collaborator

ok, I thought that was kind of the whole point about this PR. Just naming it differently but still changing the code to have this behavior doesn't make sense to me

@Grazfather
Copy link
Collaborator Author

Well gef_print buffers better, that's why it exists.

@theguy147
Copy link
Collaborator

Yeah, I didn't question the existance of gef_print but of this PR in the case that sep should behave the same in gef_print as in print but still every variadic argument should have a newline in between them.

@Grazfather
Copy link
Collaborator Author

I'm saying that the variadic arguments should NOT have a newline between them, since that makes gef_print behave more differently from print than it should. I basically forgot that print was already variadic when I wrote this.

@theguy147
Copy link
Collaborator

ah ok. so then I agree xD

@Grazfather
Copy link
Collaborator Author

Redid it so it acts like print. Now it's quite boring and we don't even really use the variadic part anywhere.

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Show resolved Hide resolved
gef.py Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

let's make gef_print great again

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated
return gef.ui.stream_buffer.write(x + kwargs.get("end", "\n"))
print(x, *args, **kwargs)
gef.ui.stream_buffer.write(
sep.join(highlight_text(a) for a in args) + end)
Copy link
Owner

Choose a reason for hiding this comment

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

sep.join(highlight_text(a) for a in args) is repeated, can be simplify imo

    text = sep.join(highlight_text(a) for a in args)
    if gef.ui.stream_buffer and not is_debug():
        gef.ui.stream_buffer.write(text+ end)
        return

    print(text, end=end, **kwargs)
    return

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's good.
Note that you can click and drag the + for a comment over multiple lines, and that way you can give a multi line suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about this then?

text = [highlight_text(a) for a in args]
if gef.ui.stream_buffer and not is_debug():
    gef.ui.stream_buffer.write(sep.join(text) + end)
else:
    print(text, sep=sep, end=end, **kwargs)

that way we are closer to how pythons builtin print works. (and IMO it would also be nice to start removing unnecessary trailing returns at the end of functions)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's better actually, though we'd need to do print(*text... otherwise the print would but [] around it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(And hugsy likes those returns because he likes how they are explicit)

Co-authored-by: hugsy <hugsy@users.noreply.github.com>
Grazfather and others added 5 commits January 20, 2022 19:54
Co-authored-by: hugsy <hugsy@users.noreply.github.com>
Co-authored-by: hugsy <hugsy@users.noreply.github.com>
Co-authored-by: hugsy <hugsy@users.noreply.github.com>
Co-authored-by: hugsy <hugsy@users.noreply.github.com>
@hugsy hugsy merged commit eaf9d11 into dev Jan 21, 2022
@hugsy hugsy deleted the variadic_gef_print branch January 21, 2022 02:07
@hugsy hugsy added this to the Release: next milestone Jan 21, 2022
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.

Add variadic params to gef_print
4 participants