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

insert(or merge) object should replace same key , not ignore #661

Closed
itviewer opened this issue Jul 21, 2017 · 12 comments
Closed

insert(or merge) object should replace same key , not ignore #661

itviewer opened this issue Jul 21, 2017 · 12 comments
Assignees
Labels
kind: enhancement/improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@itviewer
Copy link

itviewer commented Jul 21, 2017

json j1 = {{"one", 123}, {"two", 123}};
json j2 = {{"one", 456}};
j1.insert(j2.begin(), j2.end());

As key "one" has exist, now the result j1 is still {{"one", 123}, {"two", 123}}
while I think more commonly used logic is replace the same key,that is the result should be {{"one", 456}, {"two", 123}}

Thanks

@nlohmann
Copy link
Owner

This method should behave equivalent to std::map::insert: The documentation there states:

Inserts element(s) into the container, if the container doesn't already contain an element with an equivalent key.

@itviewer
Copy link
Author

itviewer commented Jul 21, 2017

It was a pity that we limited by std::map::insert does not overwrite the exist key.
I'm not familiar with c++ stl, but I often use python and js ,either python's dict or js‘s array are implemented to replace the exist key. the approach of std::map::insert is really counter-intuitive.

So what is the problem with the operator [] ?example as

void merge(json &j1, const json &j2) {
    for (json::const_iterator it = j2.begin(); it != j2.end(); ++it) {
        j1[it.key()] = it.value();
    }
}

@nlohmann
Copy link
Owner

Interestingly, there will be a std::map::merge function in C++17 which again states:

If there is an element in *this with key equivalent to the key of an element from source, then that element is not extracted from source.

@nlohmann
Copy link
Owner

From Python 3.6, we could borrow the ideas from update (emphasis added):

Update the dictionary with the key/value pairs from other, overwriting existing keys. Return None.

update() accepts either another dictionary object or an iterable of key/value pairs (as tuples or other iterables of length two). If keyword arguments are specified, the dictionary is then updated with those key/value pairs: d.update(red=1, blue=2).

So this would basically be your code with a different interface:

void merge(const_reference j)
{
    if (not (is_object() and j.is_object()))
    {
        JSON_THROW(type_error::create(305, "cannot use merge with " + type_name()));
    }

    for (auto it = j.begin(); it != j.end(); ++it) {
        m_value.object->operator[](it.key()) = it.value();
    }
}

Any thoughts on this?

@nlohmann nlohmann added kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option and removed kind: question labels Jul 21, 2017
@jaredgrubb
Copy link
Contributor

std::map::insert_or_assign basically does this, although it does not operate on a range, just one element. I would say that is the most discoverable name.

@itviewer
Copy link
Author

I think @nlohmann 's suggestion ok for now, The interface name can be either merge or insert_or_assign,but the function implementation no need to use c++ 17's std::map::insert_or_assign,until c++ 17 becomes a formal standard and is supported by mainstream compilers.

@jaredgrubb
Copy link
Contributor

Yes, I would not use the C++17 function until this library is C++17 only. It's very easy to implement the functionality. My suggestion was just about the naming.

@nlohmann
Copy link
Owner

I would implement insert_or_assign once I focus on C++ beyond C++11. I think the name is misleading if it would be used for more than a single value.

I tend to add the merge function I described in #661 (comment). However, I am unsure about the name, because std::map::merge has the semantics we do not want.

What about going the Python way and use update?

@itviewer
Copy link
Author

What about going the Python way and use update?

It's an acceptable solution

@nlohmann
Copy link
Owner

nlohmann commented Aug 1, 2017

Via Twitter, I got further proposals:

  • apply
  • combine
  • insert_or_update
  • extend

nlohmann added a commit that referenced this issue Aug 2, 2017
nlohmann added a commit that referenced this issue Aug 2, 2017
@DmitryKuk
Copy link
Contributor

upsert as an option

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed state: please discuss please discuss the issue or vote for your favorite option labels Aug 15, 2017
@nlohmann nlohmann self-assigned this Aug 15, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Aug 15, 2017
@nlohmann
Copy link
Owner

Added a functionupdate. I shall merge the feature branch once the Travis build succeeds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

4 participants