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

Improve type annotations for io.lobster.{lobsterenv/outputs} #3887

Merged
merged 66 commits into from
Aug 30, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Jun 19, 2024

Summary

  • Improve type annotations and docstring tweaks for io.lobster.{lobsterenv/outputs}.
  • Separate unit tests for lobster inputs and outputs in acc9a1f.

TODOs:

@DanielYang59 DanielYang59 changed the title Improve type annotations for io.lobster.lobsterenv Improve type annotations for io.lobster.{lobsterenv/outputs} Jun 19, 2024
@JaGeo
Copy link
Member

JaGeo commented Aug 29, 2024

@DanielYang59 I think I am fine with this. Thank you! :)

One point, however: It is really hard to see the diff on the tests. Can you just make sure everything has been moved and that all tests are still in place?

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Aug 29, 2024

@DanielYang59 I think I am fine with this. Thank you! :)

Thanks a lot for reviewing! No problem.

One point, however: It is really hard to see the diff on the tests. Can you just make sure everything has been moved and that all tests are still in place?

I'm pretty sure I didn't touch the actual content of those tests, just splitting acc9a1f. The slight difference in number of lines should be related to imports.

@JaGeo
Copy link
Member

JaGeo commented Aug 29, 2024

@DanielYang59 Great! Thank you :).

@DanielYang59
Copy link
Contributor Author

No problem, also pinging @janosh for review. Thanks!

@janosh janosh added types Type all the things lobster Lobster package (Local Orbital Basis Suite Towards Electronic-Structure Reconstruction) labels Aug 29, 2024
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

can we change the Literal doc string format to just use pipes? unless someone disagrees with that style?

- (Literal["icoop", "icobi"])
+ ("icoop" | "icobi")

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Aug 29, 2024

can we change the Literal doc string format to just use pipes?

Yep that's a good point (dropping the Literal is more concise, also just use human-readable things), Google docstring standard doesn't seem to have such detailed rules on typing yet.

I believe there will be other violations, in this case are you aware of any IDE functionality to search for something only in code/comment, which would be very useful if we want to search for Literal in comment only for example (or replace numpy with NumPy but just in comments). I didn't really find one for VScode, excepting for using regular expression but is not very straightforward.

A similar feature request microsoft/vscode#55832 but didn't seem to get implemented.

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks a lot @DanielYang59! 👍 super fast to address comments as always 😄

@janosh janosh merged commit 1f954af into materialsproject:master Aug 30, 2024
43 checks passed
@DanielYang59 DanielYang59 deleted the type-lobsterenv-outputs branch August 30, 2024 06:03
@DanielYang59
Copy link
Contributor Author

No problem. Thanks for reviewing @JaGeo (hope you had a wonderful vacation)

@DanielYang59
Copy link
Contributor Author

are you aware of any IDE functionality to search for something only in code/comment

I did some search on this, seemingly VS Code doesn't seem to have the feature or from extensions (yet), though there is a related feature request on 2016.

The good news being PyCharm does have this functionality (even for the free community version):

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lobster Lobster package (Local Orbital Basis Suite Towards Electronic-Structure Reconstruction) types Type all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants