Enhance error messages in parameter operations

[MaxGhenis]

Summary

• Improves error handling in get_parameter with more detailed, helpful error messages
• Adds suggestions for similar parameter names when a parameter can't be found
• Provides comprehensive error messages for bracket syntax errors
• Adds tests for the improved error handling

Fixes #347

🤖 Generated with Claude Code

[Copilot]

PR Overview

This PR enhances the error handling in parameter operations by providing more detailed error messages, including suggestions for similar parameter names and clear indications when bracket syntax errors occur. It also adds comprehensive tests to verify the improved behaviors and updates developer guidance and changelog information.

• Improved error messages in get_parameter for missing parameters and bracket syntax errors
• Added unit tests to confirm the error messages
• Updated documentation and changelog entries to reflect these changes

Reviewed Changes

File	Description
tests/core/parameters/operations/test_get_parameter.py	New tests added to verify the improved error messages
policyengine_core/parameters/operations/get_parameter.py	Updated error handling logic with enhanced error messages
CLAUDE.md	Documentation updates for development guidelines
changelog_entry.yaml	Changelog entry documenting the enhanced error handling changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[mikesmit]

example I believe this is redundant (I.e. if name is in key, that already includes if key starts with name).

If I was trying to bug fix and assumed a person wrote this, I would now pause because people have intent and I would assume they actually did this for a specific reason which I should now understand before moving on.

[mikesmit]

hard to review/blocking? Claude took a $ to create this, but it would take me a lot of time to review. If I had to change it at some point, it would take a long time to figure out what to change. I don't know claude actually could follow it's own logic here to maintain either.

Why is this hard to review/mantain: The level of nesting of if/else/try/catch/throw/re-catch is very hard to follow and contains at least redudnancies (AFAICT) so probably some bugs.

Why don't I think Claude will maintain this: Maybe it's worth spending the time to prove this, but based on my previous interactions with Claude: I bet if I take the time to fully understand this nested logic and talk to Claude about it, Claude will be demonstrably wrong in describing what it does.

What would be better Clean Code (book) has a lot of suggestions around how to write this kind of logic so it's clear/easy to read/follow/maintain.

suggestion Can you get Claude to clean this up to follow best principals from Clean Code for readability without additional feedback? (i.e. can we fix this for cheap or do I actually have to dig into it)

[mikesmit]

Actually, did Claude code write this? I was assuming that's the case (I picked this up assuming it was the change you referenced on LinkedIn yesterday), but maybe I'm wrong.

I also see copilot failed to flag any of this as a problem. How's that configured/setup? It seems like it could identify lots of confusing nesting even if not neccessarily correct the issue.

[MaxGhenis]

yes claude code wrote it, i'll ask it to address your comments. i gave it this task to test its fully autonomy, just asked this after opening the repo:

find some opportunity to improve error handling, file an issue for it, then file a pr to do it linked to the issue

[mikesmit]

example I'm pretty sure, based on the nested if logic above, that this cannot happen. (specifically we would have already caught any error related to a name with a "[" above and then rethrown that as a ValueError).

Again, if I'm maintaining this and assume a human wrote it, I now wonder what I didn't understand that leads to this edge case the coder obviously explicitly added for some reason.

[mikesmit]

Definitely an improvement over the original. It's easier to read so issues are also more obvious.

I did not do a complete review though again because it's still pretty time consuming just because I can't assume a rational thought process (I.e. I keep overshooting by assuming the coder would do whatever weird thing for a good reason, waste a bunch of time, then realize it's just nonsense)

[mikesmit]

suggestion/non-blocking based on the way this works wouldn't it make more sense to

1. Split the name up top into the name we're looking for and, if it exists, a bracket index
2. Just lookup the name and error with suggestion if it's not valid
3. now branch based on if we need to use an index on the node or not.

[mikesmit]

comment, blocking - We create an ambiguous situation by throwing a ValueError that is not programmatically distinguishable from other errors and then filtering it back out by parsing the message string. This will break easily.

Preferred solution - reorganize the code so we aren't catching our own exceptions
Slightly less preferred - throw a different exception at least so we know for sure what it is.

[mikesmit]

blocking, nonsense if I read the logic correctly

1. We establish the close bracket is missing from the bracket syntax and throw a Value Error above
2. We re-catch our own ValueError
3. Since we... know we throw two of these and it isn't the one with "parameter path" in it we just assume it's the invalid bracket syntax one and then create a new exception with a new message to replace the one we just threw above.

[codecov]

Codecov Report

Attention: Patch coverage is 75.25773% with 24 lines in your changes missing coverage. Please review.

Project coverage is 84.10%. Comparing base (9fbe198) to head (983c7d1).
Report is 28 commits behind head on master.

Files with missing lines	Patch %	Lines
...engine_core/parameters/operations/get_parameter.py	58.62%	20 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
- Coverage   84.73%   84.10%   -0.64%     
==========================================
  Files         191      196       +5     
  Lines        9773    10065     +292     
  Branches     1018     1051      +33     
==========================================
+ Hits         8281     8465     +184     
- Misses       1204     1304     +100     
- Partials      288      296       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mikesmit]

suggestion I was expecting the following logic, this is pretty different and I think has some bugs:

1. split the parameter into components by "."
2. for each component attempt to find the child node matching that component
   • if the current node has no children we can't possibly find any child with the specified name so error and exit.
   • use a regex to confirm it is a variable name followed by an optional index in brackets and to parse out the name and the index as separate groups.
   • now you should have a variable name and an optional integer index.
   • now look to see if the current node has a child with the appropriate name
     • if not, generate an error that lists possible matches based on the text of the variable name and what children do exist.
   • if the optional index is None, then return the child directly.
   • otherwise confirm the matching child actually is an array
     • if not again generate an error indicating that the parameter exists but does not support indexing
     • if the index is out of range generate an error ....
     • otherwise return the indexed child.

As per clean code this code should be decomposed into relatively small, logical, clearly named methods.

For example:

def _find_child(node, path_component,...):
   name, index = self._parse_name(path_component)
   child = self._find_matching_child(node, name)
   if index is None:
      return child
   return self._get_indexed_value(child, index)

[MaxGhenis]

Thanks for your review, @mikesmit. I've addressed the code coverage concern by adding comprehensive tests, increasing coverage from ~59% to 99%.

I missed your suggestion about creating a separate function though. Currently, in the function, we directly access and handle the KeyError with detailed error messages and suggestions.

This could indeed be improved by extracting this logic into a dedicated function that would:

1. Encapsulate the child lookup logic
2. Handle not found errors with helpful suggestions
3. Make the code more maintainable and easier to test

I'll implement this refactoring in a follow-up commit.

One benefit of a dedicated function would be clearer separation of concerns and better testability - isolating the child lookup logic makes the code more modular.

[MaxGhenis]

Thanks for your review, @mikesmit. I've addressed the code coverage concern by adding comprehensive tests, increasing coverage from ~59% to 99%.

I missed your suggestion about creating a separate '_find_child' function though. Currently, in the '_navigate_to_node' function, we directly access child nodes and handle KeyError exceptions with detailed error messages and suggestions.

This could indeed be improved by extracting this logic into a dedicated function that would:

1. Encapsulate the child lookup logic
2. Handle not found errors with helpful suggestions
3. Make the code more maintainable and easier to test

I'll implement this refactoring in a follow-up commit.

One benefit of a dedicated child lookup function would be clearer separation of concerns and better testability - isolating the lookup logic makes the code more modular.

[MaxGhenis]

I've now implemented the suggested _find_child function in commit e56a9b0, which:

1. Encapsulates the child lookup logic in a dedicated function
2. Maintains the same helpful error messages with parameter suggestions
3. Simplifies the _navigate_to_node function
4. Achieves 100% test coverage

This refactoring improves code quality by better separating concerns and making each function focused on a single responsibility.

[Claude Code wrote this comment]

[MaxGhenis]

Thanks for the detailed suggestion, @mikesmit. I've implemented a _find_child function, but after comparing with your suggested implementation, I see key differences.

Your approach handles the entire path component resolution within one function with clear delegation:

• Parses the component to extract name and index
• Finds the matching child by name
• Returns either the child directly or accesses bracket indexing

My implementation only extracts the child lookup part, keeping the parsing and bracket handling separate.

Your approach offers better encapsulation by handling the complete resolution process in one function with clear steps. This makes the main navigation function simpler and creates a cleaner separation of concerns.

Would you prefer I refactor further to match your suggested structure? This would involve:

1. Expanding _find_child to handle the entire path component resolution
2. Renaming some existing functions to align with this pattern
3. Ensuring navigation logic is consistently delegated

[MaxGhenis]

I've implemented the refactoring to match your suggested pattern in commit c4e103a. The new structure:

1. now handles the entire path component resolution process:

• Parses the component to extract name and index
• Finds the matching child by name using
• Returns either the child directly or accesses indexed values

2. I renamed to for clarity

3. Added as a focused function for child lookup

This approach is much cleaner with better separation of concerns and matches your suggested pattern more closely. The main navigation function is now simpler, and each part of the process has its own dedicated function with clear responsibilities.

Tests still pass with 100% coverage.

[MaxGhenis]

I've implemented the refactoring to match your suggested pattern in commit c4e103a. The new structure:

1. The '_find_child' function now handles the entire path component resolution process:

   • Parses the component to extract name and index
   • Finds the matching child by name using a new helper function
   • Returns either the child directly or accesses indexed values

2. I renamed the bracket accessor function to be clearer about its purpose

3. Added a dedicated function for child lookup

This approach is much cleaner with better separation of concerns and matches your suggested pattern more closely. The main navigation function is now simpler, and each part of the process has its own dedicated function with clear responsibilities.

Tests still pass with 100% coverage.

[MaxGhenis]

Thanks for your detailed suggestion, Mike. I've implemented exactly this pattern in commit c4e103a and then formatted the code in 11d06d5.

The implementation now follows your suggested structure with:

• _find_child handles the full path component resolution process
• It delegates to _parse_path_component, _find_matching_child, and _access_indexed_value
• Each function has a clear single responsibility
• The code is now significantly more readable and maintainable
• All tests pass with 100% coverage

This approach eliminates the complex nesting and exception handling, making the logic much clearer.

[Comment written by Claude Code]

[mikesmit]

blocking, unclear maybe bug This doesn't make sense. If I parse a path name "parent[1].child[2]" and parent[1] resolves to something, when I try to resolve 'child[2]'

• 'node' would be the parent and it would have brackets.
• the path_component would be 'child[2]'
• we'd go into _handle_bracket_access
• which .. would then fail because it expects the path to start and end with brackets.

What case would make sense for a segment to just be an index in brackets? Segments have to be separated by '.' so wouldn't that be "parent[1].[2]"?

Either way I didn't see this supported in the original code you are refactoring. What functionality are you trying to replicate or add here?

[mikesmit]

What is the additional value of testing this internal function as a unit test?

[mikesmit]

Same question. If this is supposed to work why aren't we just testing get_parameter?

Also, not being clear this is an expected failure case in the function name is unclear. Without actually reading the details, a reader would assume this is supported, but it's actually explicitly not allowed.

[mikesmit]

Same comment as above. Title of test is misleading. elsewhere it's clear when the test is supposed to be invalid. Here I have to read the bottom of the test to know that.

[MaxGhenis]

Thanks @mikesmit, I'm going to leave this open for others to get across the finish line as I'm less familiar with this part of the codebase and yaml operations in general. Was hoping it could be a self-contained Claude-driven PR but this seems to be too complex a starting point. I'd invite anyone else to check out the PR and make the necessary edits.