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

Add type annotations for source_app/utils.py #5300

Merged
merged 1 commit into from Jul 10, 2020

Conversation

pierwill
Copy link
Contributor

@pierwill pierwill commented Jun 5, 2020

Status

Ready for review.

I've posed a few questions on specific types in the comments below.

Description of Changes

This is work towards #4399.

@pierwill pierwill marked this pull request as draft June 5, 2020 20:26
@pierwill pierwill marked this pull request as ready for review June 14, 2020 21:09
redshiftzero
redshiftzero previously approved these changes Jun 18, 2020
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

looks good!

one other thought: in other parts of the server codebase we have the imports needed only for type checking in a if TYPE_CHECKING block so they're not imported at runtime - @kushaldas (or others interested) want to keep doing that or no strong thoughts? I'll approve but not merge so folks have a chance to weigh in.

@kushaldas
Copy link
Contributor

looks good!

one other thought: in other parts of the server codebase we have the imports needed only for type checking in a if TYPE_CHECKING block so they're not imported at runtime - @kushaldas (or others interested) want to keep doing that or no strong thoughts? I'll approve but not merge so folks have a chance to weigh in.

I prefer things under if TYPE_CHECKING if possible, but this is okay for me.

@pierwill
Copy link
Contributor Author

I'll take a look at how the conditional import is done elsewhere and add this in!

@eloquence
Copy link
Member

@pierwill Looks like linter is still unhappy, would you like help resolving these linter issues?

@pierwill
Copy link
Contributor Author

pierwill commented Jul 7, 2020

That would be great! I haven't had time to troubleshoot this lately...

@eloquence
Copy link
Member

No worries, @kushaldas will poke at it a bit more tomorrow.

@kushaldas
Copy link
Contributor

This fixes the issue for me.

diff --git a/securedrop/source_app/utils.py b/securedrop/source_app/utils.py
index 3e430a81f..6c72a6de9 100644
--- a/securedrop/source_app/utils.py
+++ b/securedrop/source_app/utils.py
@@ -10,10 +10,6 @@ from threading import Thread
 
 import typing
 
-if typing.TYPE_CHECKING:
-    from typing import Optional  # noqa: F401
-
-
 import i18n
 import re
 
@@ -21,6 +17,9 @@ from crypto_util import CryptoUtil, CryptoException
 from models import Source
 from sdconfig import SDConfig
 
+if typing.TYPE_CHECKING:
+    from typing import Optional  # noqa: F401
+
 
 def logged_in() -> bool:
     return 'logged_in' in session

Also add conditional import for type checking.

This commit is work towards freedomofpress#4399.
@pierwill pierwill force-pushed the pierwill-source-utils-types branch from d225bb0 to 0a902da Compare July 9, 2020 22:39
@pierwill
Copy link
Contributor Author

Squashed and linted!

Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

👍 Thanks for all the work on this @pierwill.

@rmol rmol merged commit 2aaa47d into freedomofpress:develop Jul 10, 2020
SecureDrop Team Board automation moved this from In Development to Done Jul 10, 2020
@rmol rmol added this to the 1.5.0 milestone Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants