Skip to content

+semver:minor Add PalasoImage robust helper default overrides (BL-16221)#1499

Open
andrew-polk wants to merge 1 commit into
masterfrom
BL16221_RobustExpansion
Open

+semver:minor Add PalasoImage robust helper default overrides (BL-16221)#1499
andrew-polk wants to merge 1 commit into
masterfrom
BL16221_RobustExpansion

Conversation

@andrew-polk
Copy link
Copy Markdown
Contributor

@andrew-polk andrew-polk commented Apr 30, 2026

And expand the exception types which are retried


This change is Reviewable

And expand the exception types which are retried
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

@github-actions
Copy link
Copy Markdown

Palaso Tests

     4 files  ±0       4 suites  ±0   9m 46s ⏱️ -50s
 5 095 tests ±0   4 862 ✅ +1  233 💤  - 1  0 ❌ ±0 
16 597 runs  ±0  15 878 ✅ +2  719 💤  - 2  0 ❌ ±0 

Results for commit 62b2ae5. ± Comparison against base commit 40289d1.

@andrew-polk andrew-polk marked this pull request as ready for review April 30, 2026 22:33
Copy link
Copy Markdown
Contributor

@tombogle tombogle left a comment

Choose a reason for hiding this comment

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

@tombogle reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on andrew-polk).


SIL.Windows.Forms/ImageToolbox/PalasoImage.cs line 377 at r1 (raw file):

				// Again you'd expect that if it's corrupt, it would stay that way, but
				// experimentally, it seems we can get this if the file can't be read because it is (temporarily?) locked.
				// (The text of the message reads, "File could not be read and is possible corrupted", which

typo: possibly
Since you're there, you probably should also capitalize "experimentally"

Copy link
Copy Markdown
Member

@JohnThomson JohnThomson left a comment

Choose a reason for hiding this comment

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

@JohnThomson reviewed all commit messages and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on andrew-polk).


SIL.Windows.Forms/ImageToolbox/PalasoImage.cs line 416 at r1 (raw file):

			int maxRetryAttempts = RetryUtility.kDefaultMaxRetryAttempts,
			int retryDelay = RetryUtility.kDefaultRetryDelay,
			HashSet<Type> retryOnExceptions = null)

I would be inclined to make this additionalRetryOnExceptions, and union it with the three we already know are likely. Is there any reason we would NOT want to include the types we already know can be a problem? So it makes life easier for the client, and also, a client will automatically benefit from any further ones that get added to the library.

Copy link
Copy Markdown
Member

@JohnThomson JohnThomson left a comment

Choose a reason for hiding this comment

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

One suggestion, but if you don't think it's a good idea, :lgtm:

@JohnThomson reviewed 3 files and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on andrew-polk).

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.

3 participants