Add test for behavior of bundles with cloneType. #908
Conversation
This test currently gives an inscrutable NoSuchElementException. It should be better!
|
Can you add me as collaborator, I'd like to push a proposed improvement for the |
- Add new GetReferenceException to builder - catch .get on None in emitType on aggregate - Throw partially filled in GetReferenceException -
Proposed changes to give user friendlier messages on missing cloneType
|
@ducky64 can you review this. There might be a better way, but this seemed like |
There was a problem hiding this comment.
I'm not sure this is the best / cleanest solution. Fundamentally, I think this is an autoclonetype issue, and autoclonetype should detect whether there is aliasing in its field (which I assume is the problem here?), which can be done by testing referential equality of members between the source and cloned object. This seems to be an oversight in the autoclonetype implementation, despite the existence of a whole bunch of other sanity checks.
I also think that exposing cloneType as a public API isn't a great idea, since it exposes internal details and really requires a good grasp on internals to know when to use it. I think the consensus (#765) was to introduce a Field(...) API for Bundles, which would replace the use of cloneType here as well as make all Bundle fields uniform with things like Input(...) and Output(...). It may be worth taking another look at and bumping the priority of fixing #765.
Assuming both of the above are fine, I think it's mainly a question of what the immediate (before we get a fix for #765 in) error message for field aliasing should be. I'd like to avoid the proliferation of cloneType.
I'm kind of surprised it's causing a reference exception instead of something like a rebinding exception.
|
@ducky64 to create an issue to check for referential equality of source and dest bundle fields in autoclonetype |
This test currently gives an inscrutable NoSuchElementException. It should be better!
This test currently fails. It should always fail. However, it currently fails with a totally inscrutable error message. I'm not sure what the right error message is, but we should make it better and make the test expect it.
Related issue:
Type of change: other enhancement
Impact: no functional change
Development Phase: bug report via test
Release Notes