Skip to content

Conversation

@hroest
Copy link
Collaborator

@hroest hroest commented Mar 24, 2022

Draft proposal on how to handle enums in different namespaces

Summary by CodeRabbit

  • New Features
    • Support for scoped enums with the same name in multiple C++ namespaces; added a new namespace with an additional enum.
  • Tests
    • Expanded tests to validate enum availability, member values, documentation preservation, and type-safety when converting and comparing enums.
  • Refactor
    • Adjusted enum naming and method signatures to disambiguate identical enum names across namespaces for consistent behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@hroest
Copy link
Collaborator Author

hroest commented Mar 25, 2022

@jpfeuffer it seems like now your test with regards to type safety doesnt work any more -- I can revert this and add the namespace-enum test separately, what do you think?

@jpfeuffer
Copy link
Contributor

Yes separate tests would work great I think. Thank you!

@timosachsenberg
Copy link
Collaborator

@jpfeuffer is this ready to merge?

@jpfeuffer
Copy link
Contributor

This precedes CI and needs to be updated to run the tests.

And looking at our comments, it sounds like CI might fail.

@jpfeuffer
Copy link
Contributor

@timosachsenberg Now you can see the failures. Once they are fixed, we can merge.

@timosachsenberg
Copy link
Collaborator

thanks!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 28, 2025

Walkthrough

Adds a second C++ namespace Foo2 with its own scoped enum and updates Cython declarations and tests to wrap and expose same-named enums from multiple namespaces without collisions; adjusts Foo::enumToInt signature to use the namespaced wrapper type.

Changes

Cohort / File(s) Change Summary
Tests
tests/test_code_generator.py
Extended test_enums() to assert presence and members of Foo.MyEnum and Foo2.MyEnum, verify docs and type-safety, and reorder test value creation; added docstring clarifying wrap expectations.
C++ headers
tests/test_files/enums.hpp
Added namespace Foo2 with enum class MyEnum { A, C, D }; small formatting changes and extended Foo::enumToInt to handle MyEnum::A and return default 0.
Cython declarations (.pxd)
tests/test_files/enums.pxd
Rewrote enum declarations to use namespace-prefixed public identifiers (Foo_MyEnum, Foo_MyEnum2, Foo2_MyEnum) with wrap-as to expose MyEnum in each namespace; updated Foo::enumToInt signature to accept Foo_MyEnum; added cdef cppclass Foo2.
Changelog
CHANGELOG.md
Added unreleased 0.24.0 notes describing support for same-named enums across C++ namespaces and extended key types for operator[].

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Inspect tests/test_code_generator.py assertions and docstring for correctness and coverage.
  • Verify enums.pxd namespace-qualified wrapper names, wrap-as usage, and updated enumToInt signature match C++ types.
  • Check enums.hpp enum values and enumToInt switch for completeness and consistent behavior.
  • Ensure no accidental API regressions in other test fixtures or build scripts.

Poem

I nibble on headers, nibble on pxd,
Two namespaces now, both tidy and keyed.
My enums parade with names that don't fight,
Docs kept intact, everything's bright.
🐇 Hooray — more hops in the binding delight!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title '[TEST] example for enums in namespaces' accurately describes the main changes: test files for demonstrating enum handling across different C++ namespaces.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8bd75b5 and 5349f76.

📒 Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • tests/test_code_generator.py (2 hunks)
  • tests/test_files/enums.hpp (2 hunks)
  • tests/test_files/enums.pxd (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/test_code_generator.py
  • tests/test_files/enums.hpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: test (==3.1.0, 3.10)
  • GitHub Check: lint
🔇 Additional comments (5)
CHANGELOG.md (1)

1-6: 0.24.0 changelog entry is clear and consistent

The new bullets describe the enum-namespace feature and operator[] key-type support succinctly and follow the existing changelog style; no changes needed.

tests/test_files/enums.pxd (4)

2-41: Explanatory header nicely documents the enum/namespace pattern

The top-of-file comments clearly explain the problem, the cpdef enum class + wrap-as/wrap-attach solution, and expected Python behavior, which will make this test file much easier to understand and maintain.


54-83: Foo enums follow the documented scoped-enum wrapping pattern

The Foo_MyEnum and Foo_MyEnum2 declarations correctly use cpdef enum class with explicit C++ names ("Foo::MyEnum", "Foo::MyEnum2") and wrap‑attach/wrap‑as comments, matching the documented pattern and providing clear, type‑safe wrappers.


85-104: Foo2 enum block cleanly exercises the multi-namespace case

The Foo2_MyEnum "Foo2::MyEnum" declaration, with wrap-attach: Foo2 and wrap-as: MyEnum, is a good mirror of the Foo case and provides a clear testbed for handling identically named enums from different C++ namespaces without Python-level name collisions.


45-52: I'm encountering persistent infrastructure issues that prevent me from cloning the repository to verify the namespace alignment. However, based on the review comment and the code snippet provided, I can see that the original review is raising a valid concern that warrants manual verification.

Since I cannot access the repository files to verify the C++ declarations, I'll rewrite the review comment to be more direct while preserving the core concern:


Verify C++ namespace alignment for Foo and Foo2 declarations

The Cython declarations for Foo and Foo2 (lines 45-52) lack namespace qualifiers, assuming they are global in C++. If these classes are actually declared inside a namespace in enums.hpp (e.g., namespace Foo { class MyEnum { ... } }), the declarations will fail to link. Confirm that Foo, Foo2, and enumToInt in enums.hpp are global-scope declarations, or add appropriate namespace qualifiers to the Cython cdef extern blocks if they are namespaced in C++.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8733413 and 8bd75b5.

📒 Files selected for processing (3)
  • tests/test_code_generator.py (1 hunks)
  • tests/test_files/enums.hpp (1 hunks)
  • tests/test_files/enums.pxd (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: test (==3.0.0, 3.12)
  • GitHub Check: test (==3.1.0, 3.10)
  • GitHub Check: test (==3.1.0, 3.11)
  • GitHub Check: test (==3.1.0, 3.12)
  • GitHub Check: test (==3.0.0, 3.11)
  • GitHub Check: test (==3.0.0, 3.10)
  • GitHub Check: test (>0.29.21, 3.10)
🔇 Additional comments (4)
tests/test_files/enums.hpp (1)

23-32: LGTM! Clean namespace and enum declaration.

The addition of the Foo2 namespace with its scoped enum MyEnum is syntactically correct and follows C++ best practices. The enum values (A, C, D) are properly defined within the namespace scope.

tests/test_files/enums.pxd (2)

15-21: Good use of wrap-as metadata for enum aliasing.

The enum naming convention using Foo__MyEnum with wrap-as: MyEnum metadata provides clean Python-side naming while maintaining unique internal names. This is a good approach for namespace handling.


35-49: Well-structured namespace enum declaration.

The new Foo2 namespace enum declaration follows the established pattern with proper metadata for wrapping. The documentation and member declarations are consistent with the C++ header.

tests/test_code_generator.py (1)

69-72: Excellent test coverage for namespace enum functionality.

The test assertions properly verify:

  • Both Foo.MyEnum and Foo2.MyEnum enum existence
  • Specific member access (B from original enum, D from new enum)
  • Correct placement before existing functionality tests

This ensures the new namespace enum feature is thoroughly tested.

timosachsenberg and others added 4 commits November 20, 2025 16:49
- Change from unscoped `cpdef enum` to scoped `cpdef enum class`
  to properly generate Python Enum classes with type-safe validation
- Rename enum identifiers from double underscore (Foo__MyEnum) to
  single underscore (Foo_MyEnum) for cleaner naming
- Add missing wrap-as annotation to MyEnum2 for consistency
- Fixes "redeclared" warnings for enum values at module level

The scoped enums now use isinstance() type checking instead of
numeric range validation, properly distinguishing between different
enum types with the same values.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Update CHANGELOG.md with new features for v0.24.0:
  - Support for enums with same name in different namespaces
  - Support for arbitrary key types in operator[]
- Add comprehensive documentation to enums.pxd explaining the pattern
  for wrapping namespaced enums with wrap-as annotation
- Add documentation to enums.hpp explaining the C++ structure
- Add detailed docstring to test_enums() explaining:
  - How scoped enums are mapped to Python Enum classes
  - The pattern for handling namespace conflicts
  - Type-safe enum validation behavior

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@timosachsenberg
Copy link
Collaborator

looks good to me

@timosachsenberg timosachsenberg merged commit bd34766 into OpenMS:master Nov 28, 2025
11 checks passed
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.

4 participants