remove items from sequence#582
Conversation
jbeder
left a comment
jbeder
left a comment
There was a problem hiding this comment.
Please run clang-format on this changelist, since I think some of the indenting is inconsistent.
| @@ -0,0 +1,45 @@ | |||
| #include "yaml-cpp/yaml.h" | |||
There was a problem hiding this comment.
Instead of a new demo executable, unit tests are sufficient.
| EXPECT_EQ(0, node.size()); | ||
| } | ||
|
|
||
| TEST(NodeTest, RemoveUnassignedNodeFromMap) { |
There was a problem hiding this comment.
Similarly, please add a tests that verifies that if you have a map, where one of the keys is an integer, you can remove it and it's still a map.
|
|
||
| template <typename Key, typename Enable = void> | ||
| struct remove_idx { | ||
| static char remove(std::vector<node*>& /* sequence */, const Key& /* key */) { |
There was a problem hiding this comment.
No need to comment out the variables.
| } | ||
| if (m_type == NodeType::Sequence) { | ||
| char result = remove_idx<Key>::remove(m_sequence, key); | ||
| if (result == 0) return false; |
There was a problem hiding this comment.
Add curly braces around all "if" statements, even if they're one line.
(The codebase isn't quite consistent there, but I'm trying to enforce it for new code.)
|
|
||
| template <typename Key, typename Enable = void> | ||
| struct remove_idx { | ||
| static char remove(std::vector<node*>& /* sequence */, const Key& /* key */) { |
There was a problem hiding this comment.
Why does this return a 'char'? It looks like in all implementations, it either returns 0 or 1.
| char result = remove_idx<Key>::remove(m_sequence, key); | ||
| if (result == 0) return false; | ||
| if (result == 1) return true; | ||
| convert_to_map(pMemory); |
There was a problem hiding this comment.
Can this ever happen? (See above comment.)
| convert_to_map(pMemory); | ||
| } | ||
| if (m_type == NodeType::Map) { | ||
|
|
|
@jbeder Thank you so much and I'm honoured by your reviewing .To be honest, this is the first time to create a pull request and there is many rules i need learn. I am not familiar with C++ . Haha. Thanks again and I will fix all these mistake by your review. |
|
No problem! Thanks for contributing! |
…th a integer key in a map
|
@jbeder Done |
2 participants
based on pull request 471 pullrequest 471
allow users remove items from sequence and keep the type