Skip to content
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

Improve Dict\merge #413

Closed
veewee opened this issue Jul 11, 2023 · 1 comment
Closed

Improve Dict\merge #413

veewee opened this issue Jul 11, 2023 · 1 comment
Assignees
Labels
Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Available No one has claimed responsibility for resolving this issue. Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors.

Comments

@veewee
Copy link
Collaborator

veewee commented Jul 11, 2023

Describe the bug
Currently Dict\merge is using the spread operator internally.
This behaves weird on numeric or numeric-string keys:

https://3v4l.org/gV0V8

var_dump([
    ...['1000' => 'hello', 'non-numeric' => 'world', 0 => 'you too']
]);
array(3) {
  [0]=>
  string(5) "hello"
  ["non-numeric"]=>
  string(5) "world"
  [1]=>
  string(7) "you too"
}

Expected behavior

Since the merge is being done on dict's, it should merge based on exact key and not try to merge as a list.
We already have Vec\concat for concatenating lists.

array(3) {
  ["1000"]=>
  string(5) "hello"
  ["non-numeric"]=>
  string(5) "world"
  [0]=>
  string(7) "you too"
}
@veewee veewee added the Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors. label Jul 11, 2023
@azjezz azjezz added Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Available No one has claimed responsibility for resolving this issue. labels Jul 11, 2023
@veewee
Copy link
Collaborator Author

veewee commented Jul 11, 2023

Oh sorry, opened up an issue to fast!
It seems to be working just fine - I'll create a PR with an additional testcase to prove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Available No one has claimed responsibility for resolving this issue. Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Development

No branches or pull requests

2 participants