Fix: Error when inserting an empty list in manyOf #43
Conversation
This file contains 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
The bug is described in mdgriffith#42, but I would not consider it a fix as it does not address the error message helpfullness. The problem is that in `ExpectManyOf` case of `Mark.Internal.Description.matchExpected` . There are two variables there: - `oneOptions` `twoOptions` In case of updates `oneOptions` will be a list of all valid options. The `twoOptions` will be blocks that are actually inserted. We want to check if all inserted blocks match any of the valid options. But it was the other way around - the test was "all valid options are inserted". Coincidentally when there is only one valid option and at least one block is inserted, the result is the same. That's probably why the problem remained undetected for relatively long time. I ran into it becasue my list was nested in a record (as described in the linked issue) and was initially empty.
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.
The bug is described in #42, but I would not consider this PR a fix as it does not address the error message helpfullness. Seems like more work need to be done there.
The problem this PR addresses is in the
ExpectManyOfcase ofMark.Internal.Description.matchExpected. There are two variables there:oneOptionstwoOptionsIn case of updates
oneOptionswill be a list of all valid options. ThetwoOptionswill be a list of blocks that are actually inserted.We want to check if all inserted blocks match any of the valid options. But it was the other way around - the test was "Are all valid options inserted?". Coincidentally when there is only one valid option and one block is inserted, the result is the same. That's probably why the problem remained undetected for relatively long time. I ran into it becasue my list was nested in a record (as described in the linked issue) and was initially empty.
That's also why I consider the error message unhelpful. It suggested there is something wrong with the record, but the "problem" was nested in one of the fields. I have some thoughts about improving this, but one thing at a time :D