Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

fix type annotation in get_stream_id_for_event_txn #12470

Closed
wants to merge 4 commits into from
Closed

fix type annotation in get_stream_id_for_event_txn #12470

wants to merge 4 commits into from

Conversation

gautamgiri-dev
Copy link

@gautamgiri-dev gautamgiri-dev commented Apr 13, 2022 •
edited
Loading

Fixes: #12437
StreamWorkerStore.get_stream_id_for_event_txn now has return type Optional[int] which is consistent with the NoneType when allow_none=True.

Signed-off-by: Gautam Kumar Giri personal.gautamgiri@gmail.com

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@gautamgiri-dev gautamgiri-dev requested a review from a team as a code owner April 13, 2022 19:16
@gautamgiri-dev gautamgiri-dev changed the title fixed annotation in get_stream_id_for_event_txn for #12437 fixed annotation in get_stream_id_for_event_txn for matrix-org/synapse#12437 Apr 13, 2022
@gautamgiri-dev gautamgiri-dev changed the title fixed annotation in get_stream_id_for_event_txn for matrix-org/synapse#12437 fixed annotation in get_stream_id_for_event_txn for #12437 Apr 13, 2022
@gautamgiri-dev gautamgiri-dev changed the title fixed annotation in get_stream_id_for_event_txn for #12437 fixed annotation in get_stream_id_for_event_txn for closes:#12437 Apr 13, 2022
@gautamgiri-dev gautamgiri-dev changed the title fixed annotation in get_stream_id_for_event_txn for closes:#12437 fixed type annotation in get_stream_id_for_event_txn Apr 13, 2022
@richvdh richvdh changed the title fixed type annotation in get_stream_id_for_event_txn fix type annotation in get_stream_id_for_event_txn Apr 14, 2022
@DMRobertson DMRobertson self-assigned this Apr 14, 2022
DMRobertson
DMRobertson suggested changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gautamgiri-dev. Thanks for taking this on. There are some problems I've outlined below. Can you also please run the linter script? See https://matrix-org.github.io/synapse/develop/code_style.html for instructions.

synapse/storage/database.py
Comment on lines +1563 to +1576
@overload
@classmethod
def simple_select_one_onecol_txn(
cls,
txn: LoggingTransaction,
table: str,
keyvalues: Dict[str, Any],
retcol: str,
allow_none: bool = False,
) -> Optional[int]:
ret = cls.simple_select_one_onecol_txn(cls, txn, table=table, keyvalues=keyvalues, retcol=retcol, allow_none=allow_none)
if ret:
return int(ret)
return ret
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right --- @overloads should not have an implementation; they should be for type hints only. See https://mypy.readthedocs.io/en/stable/more_types.html#function-overloading for more details.

Furthermore this is a generic function and there is no particular reason this should return an int. Additionally, the call to cls.simple_select_one_onecol_txn would infinitely recurse.

Can you explain your thinking here? What error was this attempting to address?

synapse/storage/databases/main/event_push_actions.py
@@ -191,7 +191,7 @@ def _get_unread_counts_by_receipt_txn(
stream_ordering = None

if last_read_event_id is not None:
stream_ordering = self.get_stream_id_for_event_txn( # type: ignore[attr-defined]
stream_ordering = self.get_stream_id_for_event_txn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in #12437, for this to have any chance of working we need to change the class EventPushActionsWorkerStore to inherit from StreamWorkerStore: see my point 2 there.

Note that this may end up causing runtime failures if we run into MRO problems. If that's the case, CI will detect this.

@DMRobertson DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Apr 14, 2022
@DMRobertson DMRobertson removed their assignment Apr 27, 2022
@clokep
Copy link
Member

clokep commented Jun 6, 2022

I think this got done in #12716.

@clokep clokep closed this Jun 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DMRobertson DMRobertson DMRobertson requested changes

Assignees
No one assigned
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect type annotation in get_stream_id_for_event_txn
4 participants
@gautamgiri-dev @clokep @DMRobertson @gautamdon