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
Refactoring numedit: PEP8 arguments, allow negative, type casts #636
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 6310809682
💛 - Coveralls |
penguinolog
force-pushed
the
numedit
branch
from
September 26, 2023 09:33
2dd7414
to
625f314
Compare
Old arguments not changing order, not forcing position, just defaults
changed to `None`
New arguments are enforcing keyword only and overridden by old if old is
not None.
Op di 26 sep. 2023 14:25 schreef Jason C ***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In urwid/numedit.py
<#636 (comment)>:
> + preserveSignificance: bool | None = None,
+ decimalSeparator: str | None = None,
+ *,
+ preserve_significance: bool = True,
+ decimal_separator: str = ".",
+ allow_negative: bool = False,
This appears to be a breaking change to code that used to use
preserveSignificance and decimalSeparator as a keyword arg. The following
changes should probably be made to preserve compatibility:
- Keep preserveSignificance defaulted to True
- Have preserve_significance default to None
- Assign preserve_significance = preserveSignificance if preserve_significance
== None
And the same for decimalSeparator.
Otherwise, from the code, it looks like the value of preserveSignificance
is ignored, which is the breaking part.
Either that or just leave it be and accept that the case doesn't fit the
normal style.
—
Reply to this email directly, view it on GitHub
<#636 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD2KMMZ22Y5FOOGOJLKAIQLX4LCTFANCNFSM6AAAAAA5HMZHUU>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Yeah I see. I realized what was happening right after I made the comment
and immediately deleted it. Sorry about that! Change looks good! 😀
…On Tue, Sep 26, 2023, 8:43 AM Alexey Stepanov ***@***.***> wrote:
Old arguments not changing order, not forcing position, just defaults
changed to `None`
New arguments are enforcing keyword only and overridden by old if old is
not None.
Op di 26 sep. 2023 14:25 schreef Jason C ***@***.***>:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In urwid/numedit.py
> <#636 (comment)>:
>
> > + preserveSignificance: bool | None = None,
> + decimalSeparator: str | None = None,
> + *,
> + preserve_significance: bool = True,
> + decimal_separator: str = ".",
> + allow_negative: bool = False,
>
> This appears to be a breaking change to code that used to use
> preserveSignificance and decimalSeparator as a keyword arg. The
following
> changes should probably be made to preserve compatibility:
>
> - Keep preserveSignificance defaulted to True
> - Have preserve_significance default to None
> - Assign preserve_significance = preserveSignificance if
preserve_significance
> == None
>
> And the same for decimalSeparator.
>
> Otherwise, from the code, it looks like the value of
preserveSignificance
> is ignored, which is the breaking part.
>
> Either that or just leave it be and accept that the case doesn't fit the
> normal style.
>
> —
> Reply to this email directly, view it on GitHub
> <#636 (review)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AD2KMMZ22Y5FOOGOJLKAIQLX4LCTFANCNFSM6AAAAAA5HMZHUU>
> .
> You are receiving this because you modified the open/close state.Message
> ID: ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#636 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABLDV4AW7QUCWT3RVOID6F3X4LEXPANCNFSM6AAAAAA5HMZHUU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IntEdit
to int andFloatEdit
to floatFix: #603
Fix: #432
Partial: #502
Checklist
master
orpython-dual-support
branchtox
successfully in local environment