FieldCalculationMap should include a fallback option if parsing failed#3142
Open
sbouchexbellomie-Philips wants to merge 5 commits into
Open
Conversation
WalkthroughAdds a new FieldCalculationMapParsingFallback option to configuration and updates FieldCalculationMap to consult that option when numeric conversion fails, either logging an error (value left unset) or substituting 0.0 and continuing; minor whitespace/EOF edits included. ChangesField Calculation Parsing Fallback Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 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/MigrationTools.Clients.TfsObjectModel/Tools/FieldMappingTool/FieldMaps/FieldCalculationMap.cs`:
- Around line 85-99: The TryConvertToNumeric failure block doesn't handle
unexpected Config.parsingFallback values, causing null to be added to
parameterValues; update the logic around
TryConvertToNumeric/Config.parsingFallback (in FieldCalculationMap) to
explicitly handle all FieldCalculationMapParsingFallback enum cases (RaiseError,
ResetToZero) and add a default branch that logs an error via Log.LogError
(include parameter.Value, fieldValue, source.Id and the unsupported enum value)
and returns early instead of falling through; ensure you reference the existing
symbols TryConvertToNumeric, Config.parsingFallback,
FieldCalculationMapParsingFallback, Log.LogError/Warning and parameterValues
when making the change.
In
`@src/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs`:
- Around line 25-29: Update the XML doc for the public property parsingFallback
to show the correct non-nullable default value: replace the incorrect
"<default>null</default>" with "<default>RaiseError</default>" in the summary
comments for the parsingFallback property on the FieldCalculationMapOptions
class and ensure the <summary> and <default> tags remain present and accurate
for the FieldCalculationMapParsingFallback enum usage.
🪄 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: 916c5d9c-4c53-4aff-b176-6cb1157aa412
📒 Files selected for processing (2)
src/MigrationTools.Clients.TfsObjectModel/Tools/FieldMappingTool/FieldMaps/FieldCalculationMap.cssrc/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs
MrHinsh
approved these changes
May 20, 2026
Member
|
@sbouchexbellomie-Philips id love you to take a lok at https://github.com/nkdAgility/azure-devops-migration-platform Im not saying its ready yet... but id love some feedback on architectire and intent |
auto-merge was automatically disabled
May 20, 2026 15:09
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/MigrationTools.Clients.TfsObjectModel/Tools/FieldMappingTool/FieldMaps/FieldCalculationMap.cs (1)
85-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError paths still continue and feed
nullinto the expression.Both
RaiseErroranddefaultbranchesbreak, then execution continues toparameterValues[parameter.Key] = numericValue;. That undermines the skip-on-error intent and can cascade into expression failures. Also, the unsupported-fallback log does not passConfig.parsingFallbackinto{ParsingFallback}.Proposed fix
- if (!TryConvertToNumeric(fieldValue, out var numericValue)) { + if (!TryConvertToNumeric(fieldValue, out var numericValue)) + { switch (Config.parsingFallback) { case FieldCalculationMapParsingFallback.RaiseError: Log.LogError( "FieldCalculationMap: Source field '{SourceField}' with value '{FieldValue}' is not numeric on work item {WorkItemId}. Skipping calculation.", parameter.Value, fieldValue, source.Id); - break; + return; case FieldCalculationMapParsingFallback.ResetToZero: Log.LogWarning( "FieldCalculationMap: Source field '{SourceField}' with value '{FieldValue}' is not numeric on work item {WorkItemId}. resetting the value to 0.0.", parameter.Value, fieldValue, source.Id); numericValue = 0.0; break; default: Log.LogError( "FieldCalculationMap: Unsupported parsing fallback '{ParsingFallback}' for source field '{SourceField}' on work item {WorkItemId}. Skipping calculation.", - parameter.Value, fieldValue, source.Id); - break; + Config.parsingFallback, parameter.Value, source.Id); + return; } }Also applies to: 109-109
🤖 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/MigrationTools.Clients.TfsObjectModel/Tools/FieldMappingTool/FieldMaps/FieldCalculationMap.cs` around lines 85 - 106, The error branches still fall through and then assign null into parameterValues; update the switch in the TryConvertToNumeric failure path so that RaiseError and the default/unsupported-fallback both skip processing this parameter (use continue to the next loop iteration or otherwise return/skip assignment) while the ResetToZero branch keeps numericValue = 0.0 and proceeds to assignment; also fix the default Log.LogError call to pass Config.parsingFallback (or Config.parsingFallback.ToString()) into the '{ParsingFallback}' placeholder. Apply the same fix to the analogous spot at the other location mentioned.src/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs (1)
26-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
<default>doc forparsingFallback.
<default>null</default>is inaccurate for a non-nullable enum property. Document the actual default value.Proposed fix
- /// <default>null</default> + /// <default>RaiseError</default>As per coding guidelines, "All public properties in IProcessorOptions, IToolOptions, IFieldMapOptions, IProcessorEnricherOptions, IEndpointOptions, and IEndpointEnricherOptions implementations must have accurate XML documentation comments with
and tags".
🤖 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/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs` around lines 26 - 30, The XML <default> tag for the parsingFallback property on FieldCalculationMapOptions is wrong (shows null for a non-nullable enum); update the <default> to the actual enum member used as the default (the FieldCalculationMapParsingFallback value assigned in the class constructor or the enum's implicit default if none is assigned). Locate the parsingFallback property in class FieldCalculationMapOptions and the enum FieldCalculationMapParsingFallback, determine which enum member is effectively the default (explicit assignment or the enum's first member) and replace "<default>null</default>" with "<default>ActualEnumMemberName</default>" reflecting that value.
🤖 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/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs`:
- Around line 8-10: The parsingFallback enum field currently defaults to 0
(None) when omitted from config, so ensure the runtime default is RaiseError:
either initialize the parsingFallback field/property to
FieldCalculationMapOptions.RaiseError where it is declared or modify
SetExampleConfigDefaults() to assign parsingFallback =
FieldCalculationMapOptions.RaiseError when it is null/unspecified; apply the
same change for the other identical enum fields mentioned to guarantee the
intended fail-safe behavior.
---
Duplicate comments:
In
`@src/MigrationTools.Clients.TfsObjectModel/Tools/FieldMappingTool/FieldMaps/FieldCalculationMap.cs`:
- Around line 85-106: The error branches still fall through and then assign null
into parameterValues; update the switch in the TryConvertToNumeric failure path
so that RaiseError and the default/unsupported-fallback both skip processing
this parameter (use continue to the next loop iteration or otherwise return/skip
assignment) while the ResetToZero branch keeps numericValue = 0.0 and proceeds
to assignment; also fix the default Log.LogError call to pass
Config.parsingFallback (or Config.parsingFallback.ToString()) into the
'{ParsingFallback}' placeholder. Apply the same fix to the analogous spot at the
other location mentioned.
In
`@src/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs`:
- Around line 26-30: The XML <default> tag for the parsingFallback property on
FieldCalculationMapOptions is wrong (shows null for a non-nullable enum); update
the <default> to the actual enum member used as the default (the
FieldCalculationMapParsingFallback value assigned in the class constructor or
the enum's implicit default if none is assigned). Locate the parsingFallback
property in class FieldCalculationMapOptions and the enum
FieldCalculationMapParsingFallback, determine which enum member is effectively
the default (explicit assignment or the enum's first member) and replace
"<default>null</default>" with "<default>ActualEnumMemberName</default>"
reflecting that value.
🪄 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: 018789b8-afde-497b-9e87-9b2639c6fe9a
📒 Files selected for processing (2)
src/MigrationTools.Clients.TfsObjectModel/Tools/FieldMappingTool/FieldMaps/FieldCalculationMap.cssrc/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs (1)
23-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCorrect the
<default>documentation.The
<default>tag still saysnull, but the property is initialised toRaiseErrorat line 26. Fix it to match reality.🔧 Proposed fix
/// <summary> /// Gets or sets the parsing fallback. /// </summary> -/// <default>null</default> +/// <default>RaiseError</default> public FieldCalculationMapParsingFallback parsingFallback { get; set; } = FieldCalculationMapParsingFallback.RaiseError;As per coding guidelines, "All public properties in IProcessorOptions, IToolOptions, IFieldMapOptions, IProcessorEnricherOptions, IEndpointOptions, and IEndpointEnricherOptions implementations must have accurate XML documentation comments with
and tags".
🤖 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/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs` around lines 23 - 25, The XML doc <default> for the ParsingFallback property is incorrect (it says "null") while the property ParsingFallback in FieldCalculationMapOptions is initialized to RaiseError; update the <default> tag to reflect the actual default ("RaiseError") so the <summary> and <default> accurately document the property's initial value.
🧹 Nitpick comments (1)
src/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs (1)
5-8: ⚡ Quick winDocument the public enum.
The
FieldCalculationMapParsingFallbackenum is public API but lacks XML documentation. Add a<summary>tag explaining what this fallback strategy controls and when each value applies.📝 Proposed documentation
+/// <summary> +/// Defines fallback behaviour when numeric field parsing fails during calculation. +/// </summary> public enum FieldCalculationMapParsingFallback { + /// <summary> + /// Logs an error and leaves the field value unset. + /// </summary> RaiseError, + /// <summary> + /// Logs a warning and substitutes 0.0 for the failed value. + /// </summary> ResetToZero }🤖 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/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs` around lines 5 - 8, Add XML documentation for the public enum FieldCalculationMapParsingFallback: provide a <summary> that explains this fallback strategy controls what happens when parsing a field calculation map fails or encounters invalid data, and when each strategy should be used; add <summary> (or <remarks> if helpful) on the enum and individual <summary> tags on the members RaiseError and ResetToZero clarifying that RaiseError throws or surfaces the parsing error and ResetToZero substitutes a zero/default value and continues processing.
🤖 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.
Duplicate comments:
In
`@src/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs`:
- Around line 23-25: The XML doc <default> for the ParsingFallback property is
incorrect (it says "null") while the property ParsingFallback in
FieldCalculationMapOptions is initialized to RaiseError; update the <default>
tag to reflect the actual default ("RaiseError") so the <summary> and <default>
accurately document the property's initial value.
---
Nitpick comments:
In
`@src/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs`:
- Around line 5-8: Add XML documentation for the public enum
FieldCalculationMapParsingFallback: provide a <summary> that explains this
fallback strategy controls what happens when parsing a field calculation map
fails or encounters invalid data, and when each strategy should be used; add
<summary> (or <remarks> if helpful) on the enum and individual <summary> tags on
the members RaiseError and ResetToZero clarifying that RaiseError throws or
surfaces the parsing error and ResetToZero substitutes a zero/default value and
continues processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d28b7f2-0a28-49af-a092-3b9170b5ca10
📒 Files selected for processing (2)
src/MigrationTools.Clients.TfsObjectModel/Tools/FieldMappingTool/FieldMaps/FieldCalculationMap.cssrc/MigrationTools/Tools/FieldMappingTool/FieldMaps/FieldCalculationMapOptions.cs
💤 Files with no reviewable changes (1)
- src/MigrationTools.Clients.TfsObjectModel/Tools/FieldMappingTool/FieldMaps/FieldCalculationMap.cs
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.
Added parsing fallback option
Summary by CodeRabbit
New Features
Bug Fixes