Skip to content

Conversation

@labkey-susanh
Copy link
Contributor

@labkey-susanh labkey-susanh commented Jun 11, 2025

Rationale

When addressing Issue 52098, the override of addConversionException in CoerceDataIterator was removed since we were opting to fail earlier for lookup resolution and it seemed appropriate to fail earlier for other conversions as well. Turns out we want to not fail earlier except for lookup resolution where it would be more burdensome for trigger scripts to understand how to resolve in the face of number names.

Related Pull Requests

Changes

  • Restore override and add a comment
  • Rename method for better clarity

}
}

// Ignore conversion exceptions during coercion and just pass back the original value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider referring to Issue 52098 in this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do this, but it's not actually related to the fix for Issue 52098, only mistakenly removed in the course of fixing 52098. If I hadn't made that mistake, I wouldn't have annotated this method, so I'm somewhat inclined not to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Just a contextual hook is all. I'm fine either way.

Copy link
Contributor

@labkey-jeckels labkey-jeckels left a comment

Choose a reason for hiding this comment

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

I like Nick's suggestion. Otherwise, looks good and I validated the fix.

@labkey-susanh labkey-susanh merged commit b4606f4 into develop Jun 11, 2025
21 of 25 checks passed
@labkey-susanh labkey-susanh deleted the fb_conversionExceptionCoercion branch June 11, 2025 22:22
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