Skip to content

Only convert to array types when it is possible#449

Merged
ctrueden merged 1 commit into
masterfrom
448-convertservice-eagerly-converts-stuff-to-arrays
Dec 14, 2022
Merged

Only convert to array types when it is possible#449
ctrueden merged 1 commit into
masterfrom
448-convertservice-eagerly-converts-stuff-to-arrays

Conversation

@gselzer

@gselzer gselzer commented Dec 13, 2022

Copy link
Copy Markdown
Member

This closes #448.

Instead of reporting that DefaultConverter can convert anything to an array, we only report that we can convert things that ArrayUtils.toCollection can actually convert into a Collection.

One other concern is whether the case logic might lead to action-at-a-distance errors.

@gselzer gselzer added the bug label Dec 13, 2022
@gselzer gselzer requested a review from ctrueden December 13, 2022 20:41
@gselzer gselzer self-assigned this Dec 13, 2022
@gselzer gselzer linked an issue Dec 13, 2022 that may be closed by this pull request
@gselzer gselzer force-pushed the 448-convertservice-eagerly-converts-stuff-to-arrays branch from dad61cd to 45c3330 Compare December 13, 2022 20:43
@gselzer

gselzer commented Dec 13, 2022

Copy link
Copy Markdown
Member Author

@ctrueden how do these changes fare with backwards compatibility?

@gselzer gselzer force-pushed the 448-convertservice-eagerly-converts-stuff-to-arrays branch from 45c3330 to a639bcc Compare December 13, 2022 20:46
@gselzer gselzer marked this pull request as ready for review December 13, 2022 20:48
@ctrueden ctrueden merged commit 37475f8 into master Dec 14, 2022
@ctrueden ctrueden deleted the 448-convertservice-eagerly-converts-stuff-to-arrays branch December 14, 2022 18:38
@ctrueden

Copy link
Copy Markdown
Member

how do these changes fare with backwards compatibility?

I don't foresee any problems! The tests all passed, so it must be bug-free! 🏆

ctrueden added a commit that referenced this pull request Feb 1, 2023
This test is no longer applicable, at least with the current conversion
logic that now handles arrays and collections as of #449 et al.

There is a substantial discrepancy right now between what
DefaultConverter reports as convertible via canConvert versus
what it actually *does* support converting via the convert methods.
But that's an issue for another day!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ConvertService eagerly converts stuff to arrays

2 participants