Skip to content

Start incorporating type checker to GUI file#47

Merged
ThePhilgrim merged 11 commits into
mainfrom
type_checker
Mar 27, 2021
Merged

Start incorporating type checker to GUI file#47
ThePhilgrim merged 11 commits into
mainfrom
type_checker

Conversation

@ThePhilgrim
Copy link
Copy Markdown
Owner

No description provided.

Comment thread upwatch_gui.py Outdated
@ThePhilgrim ThePhilgrim requested a review from Akuli March 22, 2021 22:56
Comment thread upwatch.py Outdated
Comment thread upwatch.py Outdated
Comment thread upwatch.py Outdated
Comment thread upwatch.py Outdated
Comment thread upwatch_gui.py Outdated
Copy link
Copy Markdown
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

Why second review: I mypyed and found from problems from upwatch.py.

Comment thread upwatch.py Outdated
Comment thread upwatch.py Outdated
Comment thread upwatch.py
@@ -86,6 +110,7 @@ def json_difference_checker(

old_job_urls = [job_post["Job Post URL"] for job_post in json_content["Job Posts"]]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Usually json_content["Job Posts"] can be None, but not in this function. Unfortunately mypy isn't clever enough to notice that, and we need to tell that to mypy manually. See python/mypy#4245

Suggested change
old_job_urls = [job_post["Job Post URL"] for job_post in json_content["Job Posts"]]
assert json_content["Job Posts"] is not None
old_job_urls = [job_post["Job Post URL"] for job_post in json_content["Job Posts"]]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Can you please specify why Mypy even needs to notice that it cannot be None? We already told mypy that "it's optionally JobPost" This means that it will be 1 or 0, and in this function it will just always be 1. Why does mypy need more info?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Also, this suggestion is marked as outdated for some reason. I cannot "commit suggestion", and it is not implemented neither in type_checker or main.

@Akuli
Copy link
Copy Markdown
Collaborator

Akuli commented Mar 23, 2021

There's also this code, but unfortunately I cant add a review comment at it:

        job_post_dict = {
            "Job Title": job_title,
            "Payment Type": job_payment_type,
            "Budget": job_budget,
            "Job Description": job_description,
            "Job Post URL": "https://upwork.com" + job_post_url,
        }

The problem is that mypy sees this as a dict with string keys and Any values, instead of noticing that this is in fact a JobPost. This leads to error like this:

upwatch.py:202: error: Argument 2 has incompatible type "List[Dict[str, Any]]"; expected "Optional[JobPost]"

To fix this, we can add a type hint : JobPost when defining the variable:

        job_post_dict: JobPost = {
            ...
        }

ThePhilgrim and others added 9 commits March 27, 2021 17:24
Co-authored-by: Akuli <akuviljanen17@gmail.com>
Co-authored-by: Akuli <akuviljanen17@gmail.com>
Co-authored-by: Akuli <akuviljanen17@gmail.com>
Co-authored-by: Akuli <akuviljanen17@gmail.com>
Co-authored-by: Akuli <akuviljanen17@gmail.com>
Co-authored-by: Akuli <akuviljanen17@gmail.com>
Co-authored-by: Akuli <akuviljanen17@gmail.com>
@ThePhilgrim ThePhilgrim merged commit 92a66ff into main Mar 27, 2021
Akuli added a commit that referenced this pull request Mar 27, 2021
@Akuli Akuli mentioned this pull request Mar 27, 2021
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.

2 participants