-
-
Notifications
You must be signed in to change notification settings - Fork 833
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
Added functionality for negative numbers in keyboard adjust. #1522
base: dev-3.x
Are you sure you want to change the base?
Conversation
✔️ Changelog found.Thank you for adding a description of the changes |
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 feature is very cool, I thought about such functionality for a long time (about 1.5 year ago) and it's in my deep backlog with low priority, thanks for trying to implement it.
We currently have an alternative way to do this using a combination of constructors (added in v3.0.0rc1 #1236), but it's not the same as what you're doing here.
So, regarding your solution, I think the design you chose is a little strange because it's not intuitive. I have another idea of what it might look like:
builder = InlineKeyboardBuilder()
# ... add buttons
builder.adjust(2, 3, ..., 5)
In this example, ...
(circular object) is used as a separator between parts, it should be specified only once per configuration and means that the left part of the values should be used for the first part of the markup, and then the right part should be used for the last part of the markup.
Technically, we can find the position of the circular object in the size list and split the positions for the first and last pieces by that configuration.
I think this is more intuitive than negative values.
Let's discuss it.
@@ -207,36 +207,71 @@ def row( | |||
) | |||
return self | |||
|
|||
def adjust(self, *sizes: int, repeat: bool = False) -> "KeyboardBuilder[ButtonType]": | |||
def _markup_constructor(self, sizes: list, buttons: list, repeat: bool) -> list: |
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.
typehint list
is not correct:
- You must specify what should actually be on this list as it is a generic type
- We still support Python 3.8, but PEP 585 was added in Python 3.9, so you should use
List[T]
from thetyping
module instead oflist[T]
:param repeat: | ||
:return: | ||
""" | ||
if sizes: |
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 solution means we can mix positive and negative values in the size configuration (e.g. 2, -2, 3, -5) and it's a strange and unexpected format, so that too needs to be taken into account and limit the movement of positive and negative values.
@@ -0,0 +1,28 @@ | |||
==== |
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.
In reStructuredText
format, it is necessary to underline the headings as long as the heading itself.
There is also the headings should not be used inside changelog, especially first-level headers, as this will break the semantics of the change history page.
[12, False, [], [10, 2]], | ||
[12, True, [3, 2, 1], [3, 2, 1, 3, 2, 1]], | ||
[12, True, [3, -1, -4], [3, 3, 1, 1, 4]], | ||
], | ||
) | ||
def test_adjust(self, count, repeat, sizes, shape): |
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.
What if we have fewer buttons than the total of all sizes?
Description
Added functionality for negative numbers in keyboard adjust.
Type of change
If the function gets a negative number in
adjust
, it applies them to the lower buttons in the same way that positive numbers affect the upper buttons. Negative numbers have a higher priority in formatting.How Has This Been Tested?
Test Configuration:
Checklist: