Fixes #2829 Allow a user supplied list of probable parameter names fo…#2831
Conversation
…r CreateExpressionFrom in ParameterValuesCreator
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 37 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR extends Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🧹 Nitpick comments (2)
src/OSPSuite.Core/Domain/Services/ParameterValuesCreator.cs (1)
105-107: Avoid recomputing expression parameter lookup per container.
expressionParametersFor(molecule, referenceExpressionProfile)is invariant across the loop and currently re-evaluated for each physical container. Compute it once beforeSelectManyto reduce repeated filtering/allocation.♻️ Suggested refactor
public IReadOnlyList<ParameterValue> CreateExpressionFrom(IContainer physicalContainer, MoleculeBuilder molecule, ExpressionProfileBuildingBlock referenceExpressionProfile) => - physicalContainer.GetAllContainersAndSelf<IContainer>(x => x.Mode.Is(ContainerMode.Physical)) - .SelectMany(container => createExpressionFrom(container, molecule, expressionParametersFor(molecule, referenceExpressionProfile))).ToList(); + createExpressionFromWithReferenceProfile(physicalContainer, molecule, referenceExpressionProfile); + +private IReadOnlyList<ParameterValue> createExpressionFromWithReferenceProfile( + IContainer physicalContainer, + MoleculeBuilder molecule, + ExpressionProfileBuildingBlock referenceExpressionProfile) +{ + var expressionParameters = expressionParametersFor(molecule, referenceExpressionProfile); + return physicalContainer.GetAllContainersAndSelf<IContainer>(x => x.Mode.Is(ContainerMode.Physical)) + .SelectMany(container => createExpressionFrom(container, molecule, expressionParameters)) + .ToList(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OSPSuite.Core/Domain/Services/ParameterValuesCreator.cs` around lines 105 - 107, CreateExpressionFrom currently calls expressionParametersFor(molecule, referenceExpressionProfile) inside the SelectMany for each container causing repeated work; compute var expressionParams = expressionParametersFor(molecule, referenceExpressionProfile) once before calling physicalContainer.GetAllContainersAndSelf<IContainer>(...) and pass that expressionParams into createExpressionFrom in the SelectMany to reuse the same lookup across all containers (adjust CreateExpressionFrom to reference expressionParams instead of calling expressionParametersFor repeatedly).tests/OSPSuite.Core.Tests/Domain/ParameterValuesCreatorSpecs.cs (1)
347-359: Prefer path/name-based assertions over positional indexing in tests.These assertions assume deterministic ordering (
_parameterValues[0],_parameterValues[1]). Matching by parameter path/name would make the test resilient to harmless ordering changes.🧪 Optional test hardening
[Observation] public void the_parameter_value_should_have_matching_formula() { - _parameterValues[0].Formula.IsExplicit().ShouldBeTrue(); - _parameterValues[1].Formula.ShouldBeNull(); + var relExp = _parameterValues.Single(x => x.Path.ToString().EndsWith($"|{Constants.Parameters.REL_EXP}")); + var someName = _parameterValues.Single(x => x.Path.ToString().EndsWith("|somename")); + relExp.Formula.IsExplicit().ShouldBeTrue(); + someName.Formula.ShouldBeNull(); } [Observation] public void the_parameter_value_should_have_matching_value() { - _parameterValues[0].Value.ShouldBeNull(); - _parameterValues[1].Value.ShouldBeEqualTo(5.0); + var relExp = _parameterValues.Single(x => x.Path.ToString().EndsWith($"|{Constants.Parameters.REL_EXP}")); + var someName = _parameterValues.Single(x => x.Path.ToString().EndsWith("|somename")); + relExp.Value.ShouldBeNull(); + someName.Value.ShouldBeEqualTo(5.0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OSPSuite.Core.Tests/Domain/ParameterValuesCreatorSpecs.cs` around lines 347 - 359, The tests the_parameter_value_should_have_matching_formula and the_parameter_value_should_have_matching_value rely on positional indexing into _parameterValues (e.g. _parameterValues[0], _parameterValues[1]); change them to locate the correct ParameterValue by a stable identifier (path or name) instead: query _parameterValues for the item whose Path or Name equals the expected parameter path/name, then assert on that item's Formula and Value properties (Formula.IsExplicit()/ShouldBeNull() and Value/ShouldBeEqualTo(5.0)) so ordering changes won't break the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OSPSuite.Core/Domain/Services/ParameterValuesCreator.cs`:
- Around line 58-64: The call to expressionParametersFor(molecule,
referenceExpressionProfile) is being recomputed for each container inside the
SelectMany; compute it once before iterating and reuse it: call
expressionParametersFor(...) into a local variable (e.g., parameters) and then
pass that variable into the SelectMany's createExpressionFrom(...) invocation
over physicalContainer.GetAllContainersAndSelf<IContainer>(x =>
x.Mode.Is(ContainerMode.Physical)) so createExpressionFrom(container, molecule,
parameters) is used instead of re-calling expressionParametersFor for each
container.
---
Nitpick comments:
In `@src/OSPSuite.Core/Domain/Services/ParameterValuesCreator.cs`:
- Around line 105-107: CreateExpressionFrom currently calls
expressionParametersFor(molecule, referenceExpressionProfile) inside the
SelectMany for each container causing repeated work; compute var
expressionParams = expressionParametersFor(molecule, referenceExpressionProfile)
once before calling physicalContainer.GetAllContainersAndSelf<IContainer>(...)
and pass that expressionParams into createExpressionFrom in the SelectMany to
reuse the same lookup across all containers (adjust CreateExpressionFrom to
reference expressionParams instead of calling expressionParametersFor
repeatedly).
In `@tests/OSPSuite.Core.Tests/Domain/ParameterValuesCreatorSpecs.cs`:
- Around line 347-359: The tests
the_parameter_value_should_have_matching_formula and
the_parameter_value_should_have_matching_value rely on positional indexing into
_parameterValues (e.g. _parameterValues[0], _parameterValues[1]); change them to
locate the correct ParameterValue by a stable identifier (path or name) instead:
query _parameterValues for the item whose Path or Name equals the expected
parameter path/name, then assert on that item's Formula and Value properties
(Formula.IsExplicit()/ShouldBeNull() and Value/ShouldBeEqualTo(5.0)) so ordering
changes won't break the tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5ce0d38-7a6b-4a03-9f4f-f0d37a4a324f
📒 Files selected for processing (2)
src/OSPSuite.Core/Domain/Services/ParameterValuesCreator.cstests/OSPSuite.Core.Tests/Domain/ParameterValuesCreatorSpecs.cs
rwmcintosh
deleted the
2829-allow-a-user-supplied-list-of-probable-parameter-names-for-createexpressionfrom-in-parametervaluescreator
branch
2 participants
Fixes #2829 allow a user supplied list of probable parameter names for createexpressionfrom in parametervaluescreator
Description
We are using the expression profiles in the project to guess parameter names for a newly created parameter values based on an expression in MoBi.
In R, we want to provide this 'reference' expression from the command line, so this implements an override where it can be referenced in the ParameterValuesCreator
Type of change
Please mark relevant options with an
xin the brackets.How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Reviewer checklist
Mark everything that needs to be checked before merging the PR.
This change requires a documentation updateabove is selectedScreenshots (if appropriate):
Questions (if appropriate):
Summary by CodeRabbit
Release Notes
New Features
Tests