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

django-stubs: Fix Promise related typing errors. #22665

Merged
merged 2 commits into from
Aug 29, 2022
Merged

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Aug 5, 2022

This discussion for this solution can be found here.

gettext_lazy returns Promise objects that is actually different from str. Methods that are present on str can also be accessed on such objects. And it behaves like a str except that it is not an instance of str.

Affected errors:

3c3
< Found 86 errors in 31 files (checked 1406 source files)
---
> Found 48 errors in 24 files (checked 1406 source files)
5,6d4
< corporate/lib/stripe.py error "Promise" has no attribute "format"  [attr-defined]
< corporate/lib/stripe_event_handler.py error "Promise" has no attribute "format"  [attr-defined]
8,15d5
< corporate/views/upgrade.py error "Promise" has no attribute "format"  [attr-defined]
< zerver/actions/message_edit.py error Argument 4 to "send_message_moved_breadcrumbs" has incompatible type "Optional[Promise]"; expected "Optional[str]"  [arg-type]
< zerver/actions/message_edit.py error Argument 7 to "send_message_moved_breadcrumbs" has incompatible type "Optional[Promise]"; expected "Optional[str]"  [arg-type]
< zerver/actions/streams.py error Argument 3 to "internal_send_stream_message" has incompatible type "Promise"; expected "str"  [arg-type]
< zerver/actions/streams.py error Argument 3 to "internal_send_stream_message" has incompatible type "Promise"; expected "str"  [arg-type]
< zerver/actions/streams.py error Argument 3 to "internal_send_stream_message" has incompatible type "Promise"; expected "str"  [arg-type]
< zerver/actions/streams.py error Argument 3 to "internal_send_stream_message" has incompatible type "Promise"; expected "str"  [arg-type]
< zerver/actions/streams.py error Argument 3 to "internal_send_stream_message" has incompatible type "Promise"; expected "str"  [arg-type]
21,23d10
< zerver/forms.py error Argument 1 to "ValidationError" has incompatible type "Promise"; expected "Union[str, ValidationError, Dict[str, Any], List[Any]]"  [arg-type]
< zerver/forms.py error Argument 1 to "ValidationError" has incompatible type "Promise"; expected "Union[str, ValidationError, Dict[str, Any], List[Any]]"  [arg-type]
< zerver/forms.py error "Promise" has no attribute "format"  [attr-defined]
57d28
< zerver/models.py error Argument "message" to "RegexValidator" has incompatible type "Promise"; expected "Optional[str]"  [arg-type]
61d31
< zerver/models.py error Incompatible return value type (got "Promise", expected "str")  [return-value]
65,68d34
< zerver/models.py error Dict entry 0 has incompatible type "int" "Promise"; expected "int" "str"  [dict-item]
< zerver/models.py error Dict entry 1 has incompatible type "int" "Promise"; expected "int" "str"  [dict-item]
< zerver/models.py error Dict entry 2 has incompatible type "int" "Promise"; expected "int" "str"  [dict-item]
< zerver/models.py error Dict entry 3 has incompatible type "int" "Promise"; expected "int" "str"  [dict-item]
71d36
< zerver/models.py error Argument 1 to "JsonableError" has incompatible type "Promise"; expected "str"  [arg-type]
81d45
< zerver/views/auth.py error "Promise" has no attribute "format"  [attr-defined]
84,85d47
< zerver/views/streams.py error Argument "topic" to "internal_prep_stream_message" has incompatible type "Promise"; expected "str"  [arg-type]
< zerver/views/users.py error Argument 1 to "JsonableError" has incompatible type "Promise"; expected "str"  [arg-type]

Screenshots and screen captures:

Self-review checklist

  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@andersk
Copy link
Member

andersk commented Aug 10, 2022

This is mostly blocked on

but if you’d like to move the import fix into a new PR, that might be worth merging sooner.

PIG208 added 2 commits August 25, 2022 15:14
This uses a more specific type `_StrPromise` to replace `Promise`
providing typing information for lazy translation strings.

In places where the callee evaluates the `_StrPromise` object in all
cases we simply force the evaluation with `str()`. This includes
`JsonableError` that ends up handled by the error handler middleware,
and `internal_send_stream_message` that depends on `check_stream_topic`,
requiring the `topic` to be evaluated anyway. In other siuations, the
callee is expected to be able to handle `StrPromise` explicitly.

Signed-off-by: Zixuan James Li <[email protected]>
@PIG208
Copy link
Member Author

PIG208 commented Aug 27, 2022

The above mentioned issue with the validator is fixed in 18a055124a2212e13c97bc1a83cc36b4e8104a97, along with the implementation of _StrPromise in django-stubs.

@andersk andersk added the integration review Added by maintainers when a PR may be ready for integration. label Aug 29, 2022
@timabbott timabbott merged commit bb9e80d into zulip:main Aug 29, 2022
@timabbott
Copy link
Member

Merged, thanks @PIG208!

@PIG208 PIG208 deleted the promise branch August 29, 2022 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when a PR may be ready for integration. size: L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants