-
Notifications
You must be signed in to change notification settings - Fork 111
Fix optional module updates #249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix optional module updates #249
Conversation
case let v as Module? where v == nil: | ||
return .none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part handles the double-optional nil
case. When the value is already set and not nil
, it's handled by the previous case case let v as Module:
case (.none, .value(let newModule)): | ||
try self.updateModule(key: key, newModule) | ||
|
||
case (.value(.module), .none): | ||
try self.updateModule(key: key, Optional<Module>.none as Any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows setting new module when current value is nil
and also resetting it back to nil
if needed
if case .none = value { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned about this deletion. While all unit tests pass, there may be edge cases I'm not aware of. But this deletion is needed to fall into switch statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks ok since there is a .none
added to the switch below
@@ -906,4 +906,43 @@ class ModuleTests: XCTestCase { | |||
// this should not throw because _freqs is not considered | |||
try rope.update(parameters: .init(), verify: .all) | |||
} | |||
|
|||
func testOptionalModuleUpdates() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice fix and thanks for the tests!
What
nil
Motivation
Consider the following example with an optional child module:
Currently, attempting to update
b
viaupdate(modules:verify:)
fails:The reason is it has double optional value inside:
Comparing to a non-optional child module:
Due to the double optional,
ModuleValue.build(value:)
maps the value to.other(nil)
instead of.none
and prevents updates from being applied correctly. This fix ensures that setting a value when the current value isnil
works as expected and resetting an optional module back tonil
is supported as well.