-
-
Notifications
You must be signed in to change notification settings - Fork 64
Conversation
Fixes #3206
Backend changes
- Adds size of dedupe index files to org storage stats
- Removes bytes from org storage stats when dedupe index is deleted if file was saved
- Adds new stats to org metrics endpoint
- Adds
awaitthat was missing in method for updating index state - Updates (and fixes) test for org metrics endpoint
Frontend changes
- Includes dedupe index files in Misc storage in dashboard storage meter
- Updates type for org metrics
| ): | ||
| """update dedupe index stats for specified collection""" | ||
| self.collections.find_one_and_update( | ||
| await self.collections.find_one_and_update( |
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.
whoa, i'm surprised this was missed before by mypy, and no warnings about it either?
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 think it's because there's no return to check the type of. mypy doesn't seem to be making sure that async methods are awaited generally, at least with how we set it up.
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.
Seems to be a contentious issue over in the mypy repo issues: python/mypy#2499
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 might require some changes in our codebase (there are lots of places where we await without doing anything with the return value of async methods) but this could help: https://mypy.readthedocs.io/en/stable/error_code_list2.html#check-that-awaitable-return-value-is-used-unused-awaitable
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.
Hm, this may have more unintended consequence, does adding a return type help generally?
793048b to
0e18f0b
Compare
|
Had to resolve some merge conflicts rebasing on main but should be set for re-review now |
ikreymer
left a comment
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.
Looks good! Thanks for additional type fixes as well!