-
Notifications
You must be signed in to change notification settings - Fork 229
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
feat: add amountMath.find #2155
Conversation
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.
Just comments so far. I need to see how this is used before I can make substantive comments.
return getStr; | ||
}; | ||
|
||
const getStr = makeGetStr(); |
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.
Top level mutable state make this module impure. Is this intentional?
if (matches.length <= 0) { | ||
matchNotFound = true; | ||
throw Error('match was not found'); | ||
} | ||
return matches; | ||
}); | ||
} catch (err) { | ||
if (matchNotFound) { | ||
// At least one searchRecord had no match | ||
return identity; |
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.
Unfortunately, for JavaScript this is probably the best way to do this. In tc39 I had suggested generalizing break <label>;
so that you could break to a label in a lexically enclosing function, giving us a non-erroneous non-local exit. It would make JS more expressive and arguably simpler at the same time, since it "simply" removes a restriction that would no longer need to be explained. Ah well.
}); | ||
doFind: (left, searchParameters) => { | ||
if (natMathHelpers.doIsGTE(left, searchParameters)) { | ||
return searchParameters; |
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 is frugal. As used by eventualWithdraw
you're withdrawing the least amount of rights that satisfies the pattern. This is opposite of the set variants, which leaves me confused about what the abstract meaning of the operation is.
matchNotFound = true; | ||
throw Error('match was not found'); | ||
} | ||
return arrayOfMatchingStrs; |
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 is anti-frugal. As used by eventualWithdraw
you're withdrawing the most amount of rights that satisfies the pattern. This is opposite of the nat variant, which leaves me confused about what the abstract meaning of the operation is.
Closing, but we can use this for design in the future. It is also linked from the epic |
This PR adds
amountMath.find
.amountMath.find
takes in aleftAmount
and asearchAmount
. It returns a new amount that represents the parts of leftAmount that "match" the value of searchAmount. If nothing matches, the identity element is returned. For example:and
This method can be used to find an amount in leftAmount using partial descriptions in searchAmount, such as string prefixes in the case of MathKind.STRING_SET or keys and values with some keys and values omitted in the case of MathKind.SET. If searchAmount.value is an array as in the case of MathKind.STRING_SET and MathKind.SET, every element of the array must have a match or the identity element is returned. Additionally, if searchAmount.value is an array, a separate search is performed for each element in searchAmount.value and the results are returned in an array with any duplicates among the searches removed.
This PR adds a number of tests, but further usages of
amountMath.find
are left out. I will follow up with a few PRs stacked on this one.Business value with further usages:
want
with regexp pattern, including string prefix #1913)Changes to setMathHelpers
It was significantly easier to implement this by reducing the flexibility of
setMathHelpers
. Instead of allowing arrays of anything comparable, I restrictedsetMathHelpers
to only operate on arrays of records with string keys.Related #1905