-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add aiken/collection/pairs.{merge}
#93
base: main
Are you sure you want to change the base?
Conversation
and a minor commit for documentation fix stdlib/lib/cardano/transaction.ak Line 205 in 0c2851f
interval.everything is a constant
|
Less -> | ||
[Pair(k, v), ..self] |
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.
That is unfortunately not possible in this context. Pairs
do not make any assumption about the ordering of keys; It's fundamentally just a list. So we cannot short-circuit merging as soon as we meet a higher key; we have to traverse it in full; always.
/// | ||
/// Notice that the key is of type `Data`. The function works with any key type, not just ByteArray. | ||
/// Unlike `dict.from_pairs() |> dict.union_with() |> dict.to_pairs()` | ||
pub fn merge( |
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.
Note that since Pairs
do not necessarily enforce uniqueness, this function becomes ambiguous:
- does it apply the merge function for ALL matching key? Only the first?
This explains the choice of naming across this module like delete_first
, delete_last
, delete_all
, to clearly indicates this behavior upfront. So in a similar spirit, I rather have a merge_first
, merge_last
and merge_all
group.
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.
Hey @KtorZ , the "need" of this merge
function came from here:
https://github.com/ariady-putra/tx_util/blob/1399949737ef1ea8c4d65b3cff7d601c25bcd725/lib/tx_util/builder/txn.ak#L484-L495
and even the merging logic itself took inspiration from repsert_by_ascending_key
, but instead of replacing, we can provide our logics.
So the idea is to enable people to do something like:
let tx = transaction.placeholder
|> add_withdrawal(cred1, v1)
...some other logics
let tx' = tx
|> add_withdrawal(cred1, v1_additional)
What's your opinion on this? Or alternative? How about merge_by_ascending_key
, since the logics are the same except for the replacing part?
PS. Idk if that makes sense to do that, but idk could be handy for certain scenarios
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.
or, add a replacing fn param at repsert_by_ascending_key
?
pub fn repsert_by_ascending_key(
self: Pairs<key, value>,
key k: key,
value v: value,
compare: fn(key, key) -> Ordering,
replace: fn(value, value) -> value
) -> Pairs<key, value> {
Add a
merge
function toaiken/collection/pairs
, similar toassets.merge
but forPairs
.The function works with any key type, not just
ByteArray
. Unlikedict.from_pairs() |> dict.union_with() |> dict.to_pairs()
, notice that the key is of typeData
: