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

Type annotations for utils #2917

Merged
merged 12 commits into from
Sep 30, 2023
Merged

Type annotations for utils #2917

merged 12 commits into from
Sep 30, 2023

Conversation

jderuiter
Copy link
Contributor

Added type annotations to bookwyrm.utils. In isni.py I changed some code to handle the case when ET.Element.find or ET.Element.text might return None. I left two TODOs in the code where there is still some decision to make to either return nothing or raise an exception.

@mouse-reeve
Copy link
Member

I'm not sure how to best resolve the conflicts on base_activity here

@hughrun
Copy link
Contributor

hughrun commented Aug 7, 2023

@jderuiter as far as I can tell the conflicts are all with your work from last week so I'm also not sure whether the intention is to replace that work or build on it. Is it possible for you to rebase your mypy-utils on top of the current main? I think that should resolve these merge conflicts.

@jderuiter
Copy link
Contributor Author

jderuiter commented Aug 7, 2023 via email

@jderuiter
Copy link
Contributor Author

The conflicts should have been resolved now.

Copy link
Contributor

@dato dato left a comment

Choose a reason for hiding this comment

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

I was just reading along and noticed these two things, in case they're useful.

bookwyrm/activitypub/base_activity.py Outdated Show resolved Hide resolved
bookwyrm/activitypub/base_activity.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hughrun hughrun left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me other than as noted. Thanks for those improvements to the isni code.

bookwyrm/activitypub/base_activity.py Outdated Show resolved Hide resolved
bookwyrm/utils/cache.py Show resolved Hide resolved
bookwyrm/utils/cache.py Show resolved Hide resolved
@hughrun hughrun merged commit e34fe9a into bookwyrm-social:main Sep 30, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants