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.

Sign up for GitHub

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

Jump to bottom

mypy: pglookout/cluster_monitor.py [BF-1560] #104

Closed
wants to merge 1 commit into from
Closed

mypy: pglookout/cluster_monitor.py [BF-1560] #104

wants to merge 1 commit into from

Conversation

Mulugruntz
Copy link
Contributor

Note for future improvements:

  • MemberState:
    • This is a very loose type as no key is mandatory. This is because it is too dangerous to impose a stricter type until we have a better test coverage, as it would change some behaviour in the code (some unconventional behaviour was detected, and it may be a bug or a feature).
  • ObserverState:
    • If we want ObserverState to accept arbitrary keys, we have three choices:
      • Use a different type (pydantic, dataclasses, etc.)
      • Use a TypedDict for static keys (connection, fetch_time) and a sub-dict for dynamic keys (received from state.json).
      • Wait for something like allow_extra to be implemented into TypedDict (unlikely) How to accept dicts with extra keys? python/mypy#4617
    • And the best solution probably lies in a mix between 1 and 2.

@Mulugruntz Mulugruntz requested a review from a team as a code owner March 22, 2023 10:57
@Mulugruntz Mulugruntz self-assigned this Mar 22, 2023
@Mulugruntz Mulugruntz changed the base branch from main to sgiffard/BF-1560_add_typing/pglookout_config March 22, 2023 11:02
@Mulugruntz Mulugruntz force-pushed the sgiffard/BF-1560_add_typing/pglookout_config branch from 9ddb56f to 5aa2bff Compare March 22, 2023 11:19
@Mulugruntz Mulugruntz force-pushed the sgiffard/BF-1560_add_typing/pglookout_cluster_monitor branch from ae44c53 to d3a43b6 Compare March 22, 2023 11:19
@Mulugruntz Mulugruntz force-pushed the sgiffard/BF-1560_add_typing/pglookout_config branch from 5aa2bff to 84737ad Compare March 22, 2023 11:28
@Mulugruntz Mulugruntz force-pushed the sgiffard/BF-1560_add_typing/pglookout_cluster_monitor branch 2 times, most recently from 0aac22d to 6526ced Compare March 22, 2023 11:36
@Mulugruntz Mulugruntz force-pushed the sgiffard/BF-1560_add_typing/pglookout_cluster_monitor branch from 6526ced to bad72c0 Compare March 22, 2023 13:06
Mulugruntz
Mulugruntz commented Mar 23, 2023
View reviewed changes
pglookout/cluster_monitor.py
# Replication info
replication_slots: list[ReplicationSlotAsDict]
replication_time_lag: float | None
min_replication_time_lag: float | None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
min_replication_time_lag: float | None
min_replication_time_lag: float

@Mulugruntz
Copy link
Contributor Author

This PR and the other ones related to type hinting will be handled in #106.

@Mulugruntz Mulugruntz closed this Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mulugruntz Mulugruntz

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Mulugruntz