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

Complete type annotation in current Python code #25

Closed
kushaldas opened this issue Sep 13, 2018 · 9 comments · Fixed by #1313
Closed

Complete type annotation in current Python code #25

kushaldas opened this issue Sep 13, 2018 · 9 comments · Fixed by #1313
Labels
epic good first issue Good for newcomers help wanted Extra attention is needed

Comments

@kushaldas
Copy link
Contributor

kushaldas commented Sep 13, 2018

Because we are writing complete new code and also in Python3, we should have type annotations in all of our code.

Edited by @redshiftzero:

Let's do this in multiple PRs to minimize disruption and review burden:

@kushaldas kushaldas changed the title Add type annotation for all new code Add type annotation for all new Python code Sep 13, 2018
@mdrose
Copy link
Contributor

mdrose commented Oct 22, 2018

Not sure how far we want to go with type annotations, but while PEP 484 is supported by Python 3.5, PEP 526 requires 3.6.

[requires]
python_version = "3.5"

@ntoll
Copy link
Contributor

ntoll commented Oct 22, 2018

I must admit, I have reservations about type annotations (basically, they don't make any difference to the running code, and I'm not even sure they make code more readable). If we were to use them to aid us check the "correctness" of our code then it's a "guarded" +1 for me, and this should be added to CI / automation in make check.

@mdrose
Copy link
Contributor

mdrose commented Oct 22, 2018

@ntoll Pretty sure automated testing is @kushaldas's intention. freedomofpress/securedrop-sdk#26

@ntoll
Copy link
Contributor

ntoll commented Oct 22, 2018

+1

@heartsucker
Copy link
Contributor

I'm renaming this ticket because as it stands it's an ongoing directive and not an actionable task.

@heartsucker heartsucker changed the title Add type annotation for all new Python code Add type annotations to current Python code and add type checks to CI / make targets Oct 23, 2018
@heartsucker
Copy link
Contributor

Before we add significant type checking in this repo, we may want to implement #1764 to make the typechecks in this repo more meaningful.

@redshiftzero redshiftzero self-assigned this Apr 15, 2019
@redshiftzero redshiftzero changed the title Add type annotations to current Python code and add type checks to CI / make targets Add type annotations to current Python code Apr 25, 2019
@redshiftzero redshiftzero removed their assignment May 1, 2019
@redshiftzero redshiftzero added the help wanted Extra attention is needed label May 4, 2019
@redshiftzero redshiftzero removed the help wanted Extra attention is needed label Nov 7, 2019
@eloquence
Copy link
Member

Discussed in backlog review, not fully resolved yet. The following files are currently targeted for strict mypy type checks:

securedrop_client/db.py \
		securedrop_client/crypto.py \
		securedrop_client/config.py \
		securedrop_client/gui/_init_.py \
		securedrop_client/resources/_init_.py \
		securedrop_client/storage.py \
		securedrop_client/queue.py \
		securedrop_client/api_jobs/_init_.py \
		securedrop_client/api_jobs/base.py \
		securedrop_client/api_jobs/downloads.py \
		securedrop_client/api_jobs/uploads.py

It would be good to generate automated coverage reports, if possible, so we can gradually get to full coverage.

@eloquence eloquence changed the title Add type annotations to current Python code Complete type in current Python code Feb 8, 2021
@eloquence eloquence changed the title Complete type in current Python code Complete type annotation in current Python code Feb 9, 2021
@eloquence eloquence added help wanted Extra attention is needed good first issue Good for newcomers labels Feb 9, 2021
@sssoleileraaa
Copy link
Contributor

The following files need to be updated to fix type annotation issues:

securedrop_client/sync.py
securedrop_client/logic.py
securedrop_client/gui/widgets.py
securedrop_client/gui/main.py
securedrop_client/app.py

Then I believe we can make the following change:

diff --git a/Makefile b/Makefile
index d3421d8..1f7816d 100644
--- a/Makefile
+++ b/Makefile
@@ -40,22 +40,12 @@ check-isort: ## Check Python import organization with isort
 
 .PHONY: mypy
 mypy: ## Run static type checker
-	@mypy --ignore-missing-imports securedrop_client
-	@# Add files that are 100% typed to the below call (eventually just the below line will run so that code without static type hints will fail CI)
 	@mypy --ignore-missing-imports \
 		--disallow-incomplete-defs \
 		--disallow-untyped-defs \
-		securedrop_client/db.py \
-		securedrop_client/crypto.py \
-		securedrop_client/config.py \
-		securedrop_client/gui/__init__.py \
-		securedrop_client/resources/__init__.py \
-		securedrop_client/storage.py \
-		securedrop_client/queue.py \
-		securedrop_client/api_jobs/__init__.py \
-		securedrop_client/api_jobs/base.py \
-		securedrop_client/api_jobs/downloads.py \
-		securedrop_client/api_jobs/uploads.py
+		*.py \
+		securedrop_client/*.py \
+		securedrop_client/**/*.py
 
 .PHONY: clean
 clean:  ## Clean the workspace of generated resources

@gonzalo-bulnes
Copy link
Contributor

I've a starting point in #1312; I'm picking this up.

legoktm pushed a commit that referenced this issue Dec 11, 2023
legoktm pushed a commit that referenced this issue Dec 11, 2023
Explicitly set packages in setup.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants