braintree: Use Final for string constants#11680
Conversation
This comment has been minimized.
This comment has been minimized.
|
Conflicts now (as expected). |
This comment has been minimized.
This comment has been minimized.
srittau
left a comment
There was a problem hiding this comment.
LGTM, apart from one personal and completely optional style remark below.
| AmEx: Final = "Apple Pay - American Express" | ||
| MasterCard: Final = "Apple Pay - MasterCard" | ||
| Visa: Final = "Apple Pay - Visa" |
There was a problem hiding this comment.
Personal opinion: These user facing strings with no further meaning to the computer should just be Final[str] = ..., while something like GatewayRejected: Final = "gateway_rejected" below makes sense.
There was a problem hiding this comment.
Also just a personal opinion: I feel it's easier to indiscriminately copy&paste from source, w/o having to think too much about what arbitrarily counts as an identifier vs a human readable string, which other contributors may not notice and follow in the same way.
There's an argument about the risk of the string changing, causing more churn, but I doubt these are likely to change without a major revision:
- braintree_python won't even remove dead code without a major version bump
- this card type, for example, is part of the result object for a transaction, it's likely users already highly depend on the value of this string for their transaction logs
…ree-string-constants
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Based on #11678 to avoid conflicts, so please merge that first
As title says. There's a bunch of classes that braintree uses like string Enums
There's a few strings that were just a bit too long, triggering Y053. although I'm not sure if, given the context they're in, it makes more sense to noqa. Take a look at this commit for comparison: cea228d