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

Merge drop and keepends kwargs #2258

Open
MrQubo opened this issue Aug 21, 2023 · 6 comments · May be fixed by #2476
Open

Merge drop and keepends kwargs #2258

MrQubo opened this issue Aug 21, 2023 · 6 comments · May be fixed by #2476
Labels
easy This could be resolved in just a couple of minutes enhancement

Comments

@MrQubo
Copy link
Contributor

MrQubo commented Aug 21, 2023

recvuntil() uses drop kwarg, while recvline() and recvlines() use keepends. keepends is a logical negation of drop, while both serve the same purpose. I find this unnecessarily confusing.

I would like to propose using the same kwarg for the purpose of those functions.

It's a matter of discussion whether drop or keepends or maybe a new different keyword is better.

The new keyword can be added while preserving the old behaviour.

@MrQubo MrQubo added the feature label Aug 21, 2023
@MrQubo MrQubo changed the title Merge drop and keepends kwargs. Merge drop and keepends kwargs Aug 21, 2023
@peace-maker
Copy link
Member

I agree it's better to have them consistent. We could just support both drop and keepends in both functions too :D
I'm in favor of drop since keepends doesn't make much sense for recvuntil.

@peace-maker peace-maker added enhancement easy This could be resolved in just a couple of minutes and removed feature labels Sep 13, 2023
@sanjitkumar2016
Copy link
Contributor

In my opinion, this one is a bit tricky because changing/removing kwargs ruins backwards compatibility for everyone using the keepends kwarg. The keepends kwarg is also used much more than the drop kwarg, so I'm not sure if this is something that a lot of users would expect. Supporting a both could also be weird/conflicting if a user sets both kwargs to the same values (even though they're supposed to be opposites). If we do want to remove the keepends kwarg, we could start off by issuing a Deprecation warning in the code and then implementing the changes in a future release, and raising an exception if keepends is used. Please let me know your thoughts!

@MrQubo
Copy link
Contributor Author

MrQubo commented Jun 13, 2024

In my opinion, this one is a bit tricky because changing/removing kwargs ruins backwards compatibility for everyone using the keepends kwarg. The keepends kwarg is also used much more than the drop kwarg, so I'm not sure if this is something that a lot of users would expect. Supporting a both could also be weird/conflicting if a user sets both kwargs to the same values (even though they're supposed to be opposites). If we do want to remove the keepends kwarg, we could start off by issuing a Deprecation warning in the code and then implementing the changes in a future release, and raising an exception if keepends is used. Please let me know your thoughts!

I agree, I had the same thoughts. There's no need to remove keepends, we can depreciate it. Also, setting both drop and keepends should result in an error in my opinion.

We could do some poll whether drop or keepends is better. But I think that drop wins, simply because it's shorter to write. Technically, there's no problem in supporting both, but I think that could potentially become source of confusion, so it's better to stick to only one.

@MrQubo
Copy link
Contributor Author

MrQubo commented Sep 28, 2024

recvline() and recvlines() have different defaults for 'keepends'. It would also be good to use the same default in both for consistency, though I'm not sure how to go about changing the default. That would be obviously not backward-compatible but printing depreciation warning for everyone using default value would be quite annoying for the users.

@MrQubo MrQubo linked a pull request Sep 28, 2024 that will close this issue
@Arusekk
Copy link
Member

Arusekk commented Sep 29, 2024

Note that .readlines might need to be compatible with file-like .readlines. Not sure if it is the case, but see what happened after refactoring term.readline for instance.

@MrQubo
Copy link
Contributor Author

MrQubo commented Sep 29, 2024

readlines() is incompatible atm, because it drops newline.
Also, it makes 0 sense for me that recvline() doesn't drop the newline, but e.g. recvline_startswith() does.
From my experience, most of the time I want newline dropped when writing exploits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy This could be resolved in just a couple of minutes enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants