-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add typehints in openlibrary.accounts module #9821
Add typehints in openlibrary.accounts module #9821
Conversation
Hi! I've been very busy for the past few days but I can continue working on this now |
01aecdc
to
8856cbe
Compare
8856cbe
to
0a86938
Compare
Also monkeytype suggests adding typehints in ReadingLog innit like so: def __init__(self, user: User | None = ...) -> None: but then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, @yivgen. Just one minor piece of substantive feedback. Great job handling the if TYPE_CHECKING
stuff, and the 'mess' between list
and List
.
I think the way you handled the case you mentioned makes sense. MonkeyType is far from perfect, sadly. :)
@@ -83,7 +83,7 @@ def batch_import(raw_data: bytes) -> BatchResult: | |||
The line numbers errors use 1-based counting because they are line numbers in a file. | |||
""" | |||
user = accounts.get_current_user() | |||
username = user.get_username() | |||
username = user.get_username() if user else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you had to make this change to get mypy
to pass, and in terms of fixing the error, I would say this is an acceptable thing to do for now. It does however expose an oversight on the part of the knucklehead who wrote this function.
What probably needs to happen here is a dependency injection-like thing, where the username is passed into batch_import
. This current implementation sets up a case where (1) it's harder to unit test batch_import
than it should be, and (2) people using the function may end up with a submitter
of None
without expecting it.
Right now, however, batch_import
is only called in one place, and there it is gated by a requirement that the account be in the /usergroup/admin
group:
openlibrary/openlibrary/plugins/openlibrary/code.py
Lines 499 to 501 in 341c890
user_key = delegate.context.user and delegate.context.user.key | |
if user_key not in _get_members_of_group("/usergroup/admin"): | |
raise Forbidden('Permission Denied.') |
So let's just leave this change for now, as user.get_username()
won't return None
the way the function is currently called`, and it will be a future task to fix this up and write some tests.
@@ -626,7 +626,7 @@ def get_recently_logged_books( | |||
return cls.fetch(logged_books) if fetch else logged_books | |||
|
|||
@classmethod | |||
def get_users_read_status_of_work(cls, username: str, work_id: str) -> str | None: | |||
def get_users_read_status_of_work(cls, username: str, work_id: str) -> int | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch here:
openlibrary=# \d bookshelves_books
Table "public.bookshelves_books"
Column | Type | Modifiers
--------------+-----------------------------+--------------------------------------
username | text | not null
work_id | integer | not null
bookshelf_id | integer | not null
edition_id | integer |
private | boolean |
updated | timestamp without time zone | default timezone('utc'::text, now())
created | timestamp without time zone | default timezone('utc'::text, now())
Thanks for this, @yivgen! |
Related to #8028
Adds typehints to openlibrary.accounts and openlibrary.plugins.upstream.mybooks modules
Technical
Testing
Screenshot
Stakeholders
@scottbarnes