-
Notifications
You must be signed in to change notification settings - Fork 74
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
Introduced multiple entities in filter for CRUD and Netconf dervices #713
Conversation
Codecov Report
@@ Coverage Diff @@
## master #713 +/- ##
==========================================
+ Coverage 78.8% 79.14% +0.34%
==========================================
Files 171 172 +1
Lines 15785 15918 +133
Branches 1431 1431
==========================================
+ Hits 12439 12599 +160
+ Misses 3003 2975 -28
- Partials 343 344 +1
Continue to review full report at Codecov.
|
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.
Thanks, @ygorelik for this change!
#include "xml_subtree_codec.hpp" | ||
|
||
using namespace std; | ||
using namespace ydk; |
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.
I think these functions need to be inside the ydk
namespace as that is the convention being followed in the rest of the code. So this line can be removed and the below line can be used:
namespace ydk {
// | ||
////////////////////////////////////////////////////////////////// | ||
|
||
#include "common_utilities.hpp" |
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.
Thanks for creating this utilities file!
return false; | ||
} | ||
|
||
string replace_xml_escape_sequences(const string& xml) |
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.
According to the codecov report, this function is not being tested. Can you please add a test case for this?
#include "service_provider.hpp" | ||
|
||
using namespace std; | ||
using namespace ydk; |
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.
As mentioned below, these functions should be inside the ydk
namespace.
namespace ydk {
#include "logger.hpp" | ||
#include "service_provider.hpp" | ||
|
||
using namespace std; |
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.
Better to avoid having using namespace
statements in header (.hpp
) files. It is ok in .cpp
files. See this link for why: https://stackoverflow.com/a/5849668
sdk/cpp/core/src/netconf_service.hpp
Outdated
@@ -34,6 +34,8 @@ | |||
#include "netconf_provider.hpp" | |||
#include "types.hpp" | |||
|
|||
using namespace std; |
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.
Suggest not to add using namespace
statements in .hpp
files. See above comment.
YLOG_ERROR("Failed to get entity node."); | ||
throw(YInvalidArgumentError{"Failed to get entity node"}); | ||
return string{}; | ||
// YLOG_ERROR("Failed to get entity node."); |
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.
It may not be safe to comment out this exception. If the entity is empty, is it may be better to raise an exception instead of silently returning empty string?
sdk/cpp/core/src/path/path.cpp
Outdated
std::free(buffer); | ||
} | ||
|
||
/*auto cdata_start = ret.find("<![CDATA["); |
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.
Suggest removing these lines instead of commenting out
sdk/cpp/tests/test_core_netconf.cpp
Outdated
|
||
// Encode results | ||
xml = s.encode(*read_result, ydk::EncodingFormat::XML, true); | ||
// cout << endl << "===== XML after encoding multiple entities:" << endl << xml << endl; |
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.
Do these commented lines need to be removed?
sdk/cpp/tests/test_core_netconf.cpp
Outdated
// Print after decoding | ||
// vector<shared_ptr<ydk::path::DataNode>> data_nodes = data_node->get_children(); | ||
// for (auto dn : data_nodes) { | ||
// print_data_node(dn); |
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.
Do these commented lines need to be removed?
Also, codacy has some comments: https://app.codacy.com/app/ydk/ydk-gen/pullRequest?prid=1434270 |
Introduced multiple entities in filter for CRUD and Netconf dervices.