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

Support more arguments in FindFilesBase #1188

Merged
merged 14 commits into from
Nov 16, 2024

Conversation

JakkuSakura
Copy link
Contributor

@JakkuSakura JakkuSakura commented Aug 27, 2024

This is an attempt to make FindFiles more useful.

It has not been tested yet

closes #1174

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.

This looks great, huge improvement! Few linting things to fix and this is ready I think.

@JakkuSakura
Copy link
Contributor Author

Just fixed linting.
one notable change is fname
mypy caught that name is defined in FactBase, thus we can't use name directly

@JakkuSakura
Copy link
Contributor Author

I'm not sure how to express args: List[str] especially it may contain QuoteString

@JakkuSakura
Copy link
Contributor Author

finally...
I also managed to enhance the tests to support dict as argument.

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.

Nice! Just the test thing (I'll move and merge if I get to it before you :)).

pyinfra/facts/util/units.py Outdated Show resolved Hide resolved
@Fizzadar Fizzadar force-pushed the extended-find-files branch from 396c304 to 5762944 Compare November 16, 2024 18:13
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.

@JakkuSakura this is really awesome, thank you! Sorry it's taken me so long to review it. I've pushed up a quick fix for the tests (also made a mess on rebase first, but fixed).

@Fizzadar Fizzadar merged commit 356add8 into pyinfra-dev:3.x Nov 16, 2024
22 checks passed
@JakkuSakura
Copy link
Contributor Author

Much appreciated! It's my first PR for pyinfra and the lining has cost me some time, but eventually figured out how to do it properly

@Fizzadar
Copy link
Member

Much appreciated! It's my first PR for pyinfra and the lining has cost me some time, but eventually figured out how to do it properly

Would love any comments/ideas/etc on how to make the experience better when contributing 🙏

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.

Make get_fact(FindFiles) more versatile
2 participants