Conversation
Unifying branches
Reviewer's GuideIntroduce shared naming tree builders and a common RecursiveSubgraphNamer protocol, refactor callers to use them, and add stereochemical substituent boundary metadata plus tests and build/config tweaks. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds a shared recursive naming protocol, standardizes naming-tree construction and shortcut metadata, preserves stereochemical substituent boundaries during rendering, adds regression tests, and updates package versioning, build exclusions, development instructions, and CI conditions. ChangesNaming and metadata flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NamingAssembly
participant RenderedSubstituentName
participant PrefixFormatting
NamingAssembly->>RenderedSubstituentName: attach optional outer-parentheses metadata
RenderedSubstituentName->>PrefixFormatting: provide rendered substituent name
PrefixFormatting->>PrefixFormatting: conditionally unwrap or preserve parentheses
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- RenderedSubstituentName stores metadata on a str subclass instance attribute, which may be surprising or fragile when values are copied or transformed; consider using a lightweight wrapper/dataclass that holds both text and boundary metadata explicitly instead of extending str.
- build_naming_tree_node rejects overlapping metadata keys at runtime, but callers currently pass ad‑hoc dicts; it may be safer to centralize the allowed metadata fields or use a typed structure so additions/renames are caught at type-check time rather than via ValueError.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- RenderedSubstituentName stores metadata on a str subclass instance attribute, which may be surprising or fragile when values are copied or transformed; consider using a lightweight wrapper/dataclass that holds both text and boundary metadata explicitly instead of extending str.
- build_naming_tree_node rejects overlapping metadata keys at runtime, but callers currently pass ad‑hoc dicts; it may be safer to centralize the allowed metadata fields or use a typed structure so additions/renames are caught at type-check time rather than via ValueError.
## Individual Comments
### Comment 1
<location path="src/openclatura/naming_protocols.py" line_range="8-17" />
<code_context>
+class RecursiveSubgraphNamer(Protocol):
</code_context>
<issue_to_address>
**suggestion:** RecursiveSubgraphNamer’s positional upstream_atom parameter may be slightly mismatched with existing implementations’ keyword-only signatures.
The protocol currently declares `upstream_atom` as positional-or-keyword before `*`, while concrete implementations make it keyword-only (`*, upstream_atom=None, ...`). Although runtime behavior is unaffected, type checkers may flag this mismatch. Please update the protocol overloads to place `upstream_atom` after `*` so it is explicitly keyword-only and consistent with the implementations.
Suggested implementation:
```python
@overload
def __call__(
self,
mol: Molecule,
start_idx: int,
exclude_atoms: set[int],
*,
upstream_atom: int | None = None,
```
The protocol likely has additional overloads, and possibly a non-overload `__call__` signature, that also accept `upstream_atom`. To fully align the protocol with the concrete implementations’ keyword-only signatures, you should:
1. Update every `__call__` overload in `RecursiveSubgraphNamer` to move `upstream_atom` after `*` (making it keyword-only) in the same way as shown in the edit.
2. Ensure the non-overload `__call__` declaration (if present) also declares `*, upstream_atom: int | None = None, ...` rather than taking `upstream_atom` positionally.
This will keep type checker expectations consistent with implementations using `*, upstream_atom=None, ...`.
</issue_to_address>
### Comment 2
<location path="src/openclatura/tests/test_analysis.py" line_range="319" />
<code_context>
+ assert name_smiles(smiles) == "(3R)-3-cyclohexylcyclohexylbenzene"
+
+
+def test_locanted_stereochemical_substituent_keeps_disambiguating_parentheses():
+ parts = AssemblyParts(
+ parent_length=6,
+ is_ring=True,
+ retained_name="benzene",
+ parent_atom_symbols_by_locant={str(locant): "C" for locant in range(1, 7)},
+ substituents=[
+ SubstituentItem(
+ name=RenderedSubstituentName(
+ "((3R)-3-cyclohexylcyclohexyl)",
+ outer_parentheses_optional=True,
+ ),
+ locants=["1"],
+ ),
+ SubstituentItem(name="methyl", locants=["4"]),
+ ],
+ )
+
+ assert assemble_name(parts) == "1-((3R)-3-cyclohexylcyclohexyl)-4-methylbenzene"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a case where a stereochemical substituent is itself used as a substituent to verify boundary preservation
These tests cover optional outer parentheses on stereochemical substituents attached to the parent. `_omit_optional_outer_parentheses` also handles the case where `parts.is_substituent` is `True`: when a `RenderedSubstituentName` is itself reused as a substituent, its outer parentheses must be kept. Please add a test that builds such an `AssemblyParts` (with `is_substituent=True` and a `RenderedSubstituentName` having `outer_parentheses_optional=True`) and asserts that the parentheses are preserved in this nested substituent scenario.
```suggestion
assert name_smiles(smiles) == "(3R)-3-cyclohexylcyclohexylbenzene"
def test_stereochemical_substituent_reused_as_substituent_preserves_optional_outer_parentheses():
parts = AssemblyParts(
parent_length=2,
is_ring=False,
is_substituent=True,
retained_name="ethyl",
parent_atom_symbols_by_locant={str(locant): "C" for locant in range(1, 3)},
substituents=[
SubstituentItem(
name=RenderedSubstituentName(
"((3R)-3-cyclohexylcyclohexyl)",
outer_parentheses_optional=True,
),
locants=["1"],
)
],
)
assert assemble_name(parts) == "1-((3R)-3-cyclohexylcyclohexyl)ethyl"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_single_unlocanted_stereochemical_substituent_omits_optional_outer_parentheses(): | ||
| smiles = "C1(CCCCC1)[C@H]1CC(CCC1)C1=CC=CC=C1" | ||
|
|
||
| assert name_smiles(smiles) == "(3R)-3-cyclohexylcyclohexylbenzene" |
There was a problem hiding this comment.
suggestion (testing): Add a case where a stereochemical substituent is itself used as a substituent to verify boundary preservation
These tests cover optional outer parentheses on stereochemical substituents attached to the parent. _omit_optional_outer_parentheses also handles the case where parts.is_substituent is True: when a RenderedSubstituentName is itself reused as a substituent, its outer parentheses must be kept. Please add a test that builds such an AssemblyParts (with is_substituent=True and a RenderedSubstituentName having outer_parentheses_optional=True) and asserts that the parentheses are preserved in this nested substituent scenario.
Suggested change
| assert name_smiles(smiles) == "(3R)-3-cyclohexylcyclohexylbenzene" | |
| assert name_smiles(smiles) == "(3R)-3-cyclohexylcyclohexylbenzene" | |
| def test_stereochemical_substituent_reused_as_substituent_preserves_optional_outer_parentheses(): | |
| parts = AssemblyParts( | |
| parent_length=2, | |
| is_ring=False, | |
| is_substituent=True, | |
| retained_name="ethyl", | |
| parent_atom_symbols_by_locant={str(locant): "C" for locant in range(1, 3)}, | |
| substituents=[ | |
| SubstituentItem( | |
| name=RenderedSubstituentName( | |
| "((3R)-3-cyclohexylcyclohexyl)", | |
| outer_parentheses_optional=True, | |
| ), | |
| locants=["1"], | |
| ) | |
| ], | |
| ) | |
| assert assemble_name(parts) == "1-((3R)-3-cyclohexylcyclohexyl)ethyl" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openclatura/component_namer.py`:
- Around line 601-612: Update _component_shortcut_tree to accept mol as its
first argument and pass _bond_ids_within(mol, component_atoms) to
build_shortcut_tree_node as bond_ids. Update all three component shortcut call
sites, including the single-atom, structural-replacement-parent, and
anhydride-component paths, to pass mol first while preserving their existing
component, binding, and token-span arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e01092c-4844-4ea6-97ae-67e26dd0687c
📒 Files selected for processing (15)
.github/workflows/ci.ymlREADME.mdpyproject.tomlsrc/openclatura/assembly_parts.pysrc/openclatura/assembly_prefixes.pysrc/openclatura/component_modifiers.pysrc/openclatura/component_namer.pysrc/openclatura/functional_prefixes.pysrc/openclatura/heteroatom_subgraphs.pysrc/openclatura/namer.pysrc/openclatura/naming_protocols.pysrc/openclatura/special_cases.pysrc/openclatura/substituent_tokens.pysrc/openclatura/tests/test_analysis.pysrc/openclatura/trace_helpers.py
Comment on lines
+601
to
+612
| def _component_shortcut_tree( | ||
| name: str, component_atoms: set[int], bindings: list[dict], token_spans: list[dict] | ||
| ) -> dict: | ||
| """Return a minimal component tree for shortcut component names.""" | ||
|
|
||
| return { | ||
| "kind": "component", | ||
| "name": name, | ||
| "atoms": sorted(component_atoms), | ||
| "bonds": [], | ||
| "parent": None, | ||
| "principal_group": None, | ||
| "substituents": [], | ||
| "replacement_prefixes": [], | ||
| "unsaturations": [], | ||
| "trace_segments": [], | ||
| "nested_decisions": [], | ||
| "name_atom_bindings": bindings, | ||
| "name_token_spans": token_spans, | ||
| } | ||
| return build_shortcut_tree_node( | ||
| kind="component", | ||
| name=name, | ||
| atom_ids=component_atoms, | ||
| name_atom_bindings=bindings, | ||
| name_token_spans=token_spans, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_component_shortcut_tree never populates bond_ids, unlike its substituent-side sibling.
namer.py's _shortcut_substituent_tree passes bond_ids=_bond_ids_within(mol, component) into build_shortcut_tree_node, but this component-side counterpart omits bond_ids entirely, so the resulting tree node's "bonds" field is always []. This is harmless for the single-atom shortcut path (line 279) but drops real bond data for the structural-replacement-parent (line 323) and anhydride-component (line 378) shortcuts, which can span multiple bonded atoms (e.g. biphenyl, anhydride halves).
🔧 Proposed fix
-def _component_shortcut_tree(
- name: str, component_atoms: set[int], bindings: list[dict], token_spans: list[dict]
+def _component_shortcut_tree(
+ mol: Molecule, name: str, component_atoms: set[int], bindings: list[dict], token_spans: list[dict]
) -> dict:
"""Return a minimal component tree for shortcut component names."""
return build_shortcut_tree_node(
kind="component",
name=name,
atom_ids=component_atoms,
+ bond_ids=bond_ids_within(mol, component_atoms),
name_atom_bindings=bindings,
name_token_spans=token_spans,
)And update the three call sites (lines 279, 283, 323, 327, 378, 382) to pass mol as the first argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openclatura/component_namer.py` around lines 601 - 612, Update
_component_shortcut_tree to accept mol as its first argument and pass
_bond_ids_within(mol, component_atoms) to build_shortcut_tree_node as bond_ids.
Update all three component shortcut call sites, including the single-atom,
structural-replacement-parent, and anhydride-component paths, to pass mol first
while preserving their existing component, binding, and token-span arguments.
…tName-dataclass frozen RenderedSubstituentName dataclass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce shared recursive naming protocols and tree-node builders while improving stereochemical substituent formatting and updating packaging and CI configuration.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Summary by CodeRabbit