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

V3: type hints for operations #1082

Merged
merged 17 commits into from
Jun 8, 2024

Conversation

stone-w4tch3r
Copy link
Contributor

@stone-w4tch3r stone-w4tch3r commented Apr 5, 2024

Type hints for all operations, V3 version

@stone-w4tch3r
Copy link
Contributor Author

Looks like some check are broken :(

@stone-w4tch3r stone-w4tch3r marked this pull request as ready for review April 5, 2024 14:09
@Fizzadar
Copy link
Member

Fizzadar commented Apr 7, 2024

@stone-w4tch3r thank you so much for working on this! At a quick glance it looks fantastic, time is in extremely short supply right now I won’t be able to properly review till next weekend unfortunately, just a heads up!

Copy link
Member

@Fizzadar Fizzadar left a comment

Choose a reason for hiding this comment

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

OK that week turned into a month, my apologies @stone-w4tch3r. PR looks great, HUGE thank you for working on this!

I think the failures are mostly missing Optionals since mypy switched to disallowing implicit optional kwargs. Looks like there's a tool to automatically fix this.

I can clean those up and get that working with the tests, all the types look good 👍

@@ -25,16 +39,19 @@ def boolean(bool_name, value, persistent=False):
selinux.boolean(
name='Allow Apache to connect to LDAP server',
'httpd_can_network_connect',
'on',
Boolean.ON,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Boolean.ON,
selinux.Boolean.ON,

@@ -127,12 +144,15 @@ def port(protocol, port_num, se_type=None, present=True):

selinux.port(
name='Allow Apache to provide service on port 2222',
'tcp',
Protocol.TCP,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Protocol.TCP,
selinux.Protocol.TCP,

@Fizzadar
Copy link
Member

Rebased ontop of v3, hoping to get remaining mypy errors cleaned up this week.

Fizzadar and others added 17 commits June 8, 2024 12:06
# Conflicts:
#	pyinfra/facts/util/packaging.py
#	pyinfra/operations/apt.py
# Conflicts:
#	pyinfra/operations/dnf.py
#	pyinfra/operations/util/packaging.py
#	pyinfra/operations/yum.py
# Conflicts:
#	pyinfra/operations/files.py
#	pyinfra/operations/util/files.py
# Conflicts:
#	pyinfra/operations/gem.py
#	pyinfra/operations/git.py
Mostly just adding `| None` to all the optional operation arguments.
Copy link
Member

@Fizzadar Fizzadar left a comment

Choose a reason for hiding this comment

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

Got there in the end! Massive thank you @stone-w4tch3r for doing the majority of the work here, truly a fantastic achievement to land this in v3!

@Fizzadar Fizzadar merged commit 3d7e3cc into pyinfra-dev:3.x Jun 8, 2024
22 checks passed
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.

None yet

2 participants