Fix optional module updates#249
Merged
davidkoski merged 2 commits intoml-explore:mainfrom Jun 15, 2025
Merged
Conversation
petrukha-ivan
commented
Jun 12, 2025
Comment on lines
+1272
to
+1273
| case let v as Module? where v == nil: | ||
| return .none |
Contributor
Author
There was a problem hiding this comment.
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:
petrukha-ivan
commented
Jun 12, 2025
Comment on lines
+690
to
+694
| 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) |
Contributor
Author
There was a problem hiding this comment.
This allows setting new module when current value is nil and also resetting it back to nil if needed
petrukha-ivan
commented
Jun 12, 2025
Comment on lines
-597
to
-599
| if case .none = value { | ||
| return | ||
| } |
Contributor
Author
There was a problem hiding this comment.
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.
Collaborator
There was a problem hiding this comment.
this looks ok since there is a .none added to the switch below
davidkoski
reviewed
Jun 15, 2025
| try rope.update(parameters: .init(), verify: .all) | ||
| } | ||
|
|
||
| func testOptionalModuleUpdates() throws { |
davidkoski
approved these changes
Jun 15, 2025
Collaborator
davidkoski
left a comment
There was a problem hiding this comment.
nice fix and thanks for the tests!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
nilMotivation
Consider the following example with an optional child module:
Currently, attempting to update
bviaupdate(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.noneand prevents updates from being applied correctly. This fix ensures that setting a value when the current value isnilworks as expected and resetting an optional module back tonilis supported as well.