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

Add more docstrings and type hints #1641

Merged
merged 21 commits into from
Sep 8, 2024
Merged

Add more docstrings and type hints #1641

merged 21 commits into from
Sep 8, 2024

Conversation

jan-janssen
Copy link
Member

@jan-janssen jan-janssen commented Sep 8, 2024

This is the second part following #1640

@jan-janssen jan-janssen marked this pull request as draft September 8, 2024 09:02
@jan-janssen jan-janssen marked this pull request as ready for review September 8, 2024 10:46
@samwaseda
Copy link
Member

I’m not sure if I’d import argparse just for the type hinting. Don’t you want to do a string type hinting?

@jan-janssen
Copy link
Member Author

I’m not sure if I’d import argparse just for the type hinting. Don’t you want to do a string type hinting?

In pyiron_base.cli.control the argparse package is required to parse the command line arguments. The other command line interfaces in pyiron_base.cli.* are only imported in pyiron_base.cli.control, so it does not matter if we already import argparse in the command line interfaces of in pyiron_base.cli.control, the number of modules imported when using one of the command line utilities remains the same.

Copy link
Member

@samwaseda samwaseda left a comment

Choose a reason for hiding this comment

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

Just a nit otherwise it looks good to me

pyiron_base/database/generic.py Outdated Show resolved Hide resolved
@jan-janssen jan-janssen merged commit 23dec95 into main Sep 8, 2024
26 checks passed
@jan-janssen jan-janssen deleted the copilot branch September 8, 2024 17:07
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