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

Type hints for functions #281

Open
jwreep opened this issue May 3, 2024 · 4 comments
Open

Type hints for functions #281

jwreep opened this issue May 3, 2024 · 4 comments
Labels
discussion Issues that require some extended deliberation

Comments

@jwreep
Copy link
Collaborator

jwreep commented May 3, 2024

The return statements in functions in fiasco can be quite varied, both in terms of the units and the data types/structures. There are still a good number of functions without type hints, so I think it might be worth going through and adding type hints to improve readability of the code.

Any thoughts?

@wtbarnes
Copy link
Owner

wtbarnes commented May 3, 2024

Just to clarify, by type hints do you mean adding unit annotations to the inputs and outputs or add type hints for the data types of the inputs and outputs (eg list, int, etc)?

I ask because I'm all in favor of the former, but I'm very wary of the latter.

@jwreep
Copy link
Collaborator Author

jwreep commented May 3, 2024

Both. Why are you wary of the latter?

@wtbarnes
Copy link
Owner

wtbarnes commented May 3, 2024

I find the addition of type hints/annotations makes Python code more cluttered and less readable. The value added by having these annotations does not seem to justify the cost. However, I recognize this is largely personal preference and I recognize I'm increasingly in the minority.

If you find it helpful, it's worth considering. Did you have a particular function/method in mind where this would be useful?

@jwreep
Copy link
Collaborator Author

jwreep commented May 3, 2024

I don't have particularly strong opinions one way or other, so I'm not going to push about it. Certainly units would be helpful though.

I was looking at the ion collections (thinking of working on #232) and noticed that the functions there are missing type hints. E.g.

def free_free(self, wavelength: u.angstrom):

def spectrum(self, density: u.cm**(-3), emission_measure: u.cm**(-5), wavelength_range=None,
bin_width=None, kernel=None, **kwargs):

@wtbarnes wtbarnes added the discussion Issues that require some extended deliberation label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issues that require some extended deliberation
Projects
None yet
Development

No branches or pull requests

2 participants