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

Mark pywavemap as a typed extension and address cpplint v2.0 warnings #82

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

victorreijgwart
Copy link
Member

@victorreijgwart victorreijgwart commented Oct 24, 2024

Description

This PR adds a py.typed marker file to indicate that the pywavemap package comes with type annotations. It also addresses warnings about missing includes for STL header files generated by the new version of cpplint.

Many thanks to @Divelix for implementing it!

Type of change

  • New feature

Detailed summary

Indicating that pywavemap comes with type annotations is useful as it allows mypy, and IDEs like VSCode, to automatically detect potential issues and provide better autocompletions. For more information on py.typed files, see PEP 561. We generate the file using nanobind's CMake interface.

Regarding cpplint, we were until recently using version 1.6.1, which only detected missing includes for STL headers up to C++11. Among other improvements, version 2.0.0 also significantly increases its include-what-you-use check coverage -- especially new C++17 STL headers. This PR addresses all these new warnings.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any required changes in dependencies have been committed and pushed

@victorreijgwart victorreijgwart self-assigned this Oct 24, 2024
@victorreijgwart victorreijgwart added the enhancement New feature or request label Oct 24, 2024
@victorreijgwart victorreijgwart changed the title Indicate pywavemap is a typed extension by adding "py.typed" marker file Indicate pywavemap is a typed extension and add missing includes detected by cpplint v2.0.0 Oct 24, 2024
@victorreijgwart victorreijgwart changed the title Indicate pywavemap is a typed extension and add missing includes detected by cpplint v2.0.0 Indicate pywavemap is a typed extension and address new cpplint v2.0.0 warnings Oct 24, 2024
@victorreijgwart victorreijgwart changed the title Indicate pywavemap is a typed extension and address new cpplint v2.0.0 warnings Mark pywavemap as a typed extension and address cpplint v2.0 warnings Oct 24, 2024
Copy link
Contributor

@LionelOtt LionelOtt left a comment

Choose a reason for hiding this comment

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

Looks good to me, though probably in the future it may make sense to have different things as different PR, like linter and nanobind stuff here. Though obviously I can see with the nanobind thing being a single line it may look at bit silly :-)

@LionelOtt LionelOtt merged commit d0d64d8 into main Oct 25, 2024
25 checks passed
@victorreijgwart
Copy link
Member Author

victorreijgwart commented Oct 25, 2024

Thanks for reviewing @LionelOtt! I agree these changes should have been separate. They mainly got tangled because I didn't pin the version of cpplint used in CI, and its version 2 got released in between wavemap v2.1.0 and this PR. So I addressed its warnings to get CI to pass.

I'll add a TODO to pin the versions for all the tools we use, both in CI and in the install script we provide to set up local dev environments.

@victorreijgwart victorreijgwart deleted the feature/add_py_typed_marker_file branch November 8, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants