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

Upgrade flake8 use typedecl 2 #340

Merged
merged 5 commits into from
Feb 7, 2023
Merged

Conversation

hategan
Copy link
Collaborator

@hategan hategan commented Jan 27, 2023

flake8 6.0.0 (and corresponding pyflakes) removed support for # type: abc, which caused unused import errors. The solution was to change those type declarations to the "proper" format (var: <type> = <value>).

This time on top of main.

RamonAra209 and others added 5 commits January 11, 2023 10:11
* tests: added llnl specific tests from getting-started documentation

* tests: divided llnl doc tests into subfolder within tests

* linting: applied type annotations

* tests: Added doc tests with executor parameters

* deleted: old folder of slurm/lsf specific tests

* getting started doc tests: fixed status callback test

* addressing linting

* addressing linting
The reason why flak8 was complaining is because of the removal of support
for `# type: xyz` style declarations.
So convert all those to proper type declarations.
Copy link
Collaborator

@andre-merzky andre-merzky left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

self.kill_flag = False
self.process = None # type: Optional[subprocess.Popen[bytes]]
self.process: Optional[subprocess.Popen[bytes]] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's an interesting one - haven't seen that documented as type. Where did you find this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/subprocess.html#subprocess.Popen

It turns out Popen is a class. You'd think it's something like Open returning a File or, you know, Verb applied to Noun produces Noun, but no.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, sorry, I wasn't clear. Yes, Popen is a class - but classname[bytes] is not a syntax I have seen anywhere before.

I found one google hit by now which indicates that the Popen class is marked as generic - wasn't aware of that...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. Well, if you don't put the type parameters, mypy will produce an error. I'm not entirely sure where I found what that parameter is (str/bytes based on whether you open the output streams in text or binary mode), since I can't find a clear statement of that in the official python documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thx, good to know.

@hategan hategan merged commit 0b3c0d7 into main Feb 7, 2023
@hategan hategan deleted the upgrade_flake8_use_typedecl_2 branch February 7, 2023 18:17
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.

3 participants