-
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
Improve documentation, fix error messages #622
Conversation
ghost
commented
Nov 9, 2017
- Improve documentation. Add read+delete examples.
- Fix error messages to make it more clear
- Fix cmake flags to remove clang hardcoding & coverage
Codecov Report
@@ Coverage Diff @@
## master #622 +/- ##
==========================================
- Coverage 73.35% 73.34% -0.02%
==========================================
Files 105 105
Lines 10386 10388 +2
Branches 1450 1450
==========================================
Hits 7619 7619
- Misses 2488 2490 +2
Partials 279 279
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 for the changes. I just had a few questions.
@@ -224,7 +224,7 @@ std::shared_ptr<path::DataNode> NetconfSession::handle_netconf_operation(path::R | |||
} | |||
if(reply.find("<ok/>") == std::string::npos) | |||
{ | |||
YLOG_ERROR("No ok in reply "); | |||
YLOG_ERROR("The reply from the device is not OK"); |
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 actually think this wording is more confusing. Maybe say something like 'Did not receive "OK" reply from the device'. And maybe it would be good to include the actual reply from the device as a YLOG_DEBUG message.
@@ -388,7 +388,7 @@ static std::shared_ptr<path::DataNode> handle_edit_reply(string reply, NetconfCl | |||
{ | |||
if(reply.find("<ok/>") == std::string::npos) | |||
{ | |||
YLOG_ERROR("No ok in reply received from device"); | |||
YLOG_ERROR("The reply from the device is not OK"); |
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.
Same as above
@@ -19,12 +19,12 @@ set(samples bgp_create | |||
bgp_xr_read | |||
bgp_xr_opendaylight | |||
bgp_restconf | |||
xe_native_read) |
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.
comment out instead
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.
Done
sdk/python/core/docsgen/faq.rst
Outdated
|
||
Also, we use the `ydk-gen <https://github.com/CiscoDevNet/ydk-gen>`_ tool to generate the bundles. This tool is available for anyone to use in order to generate the bundle version in combination with ydk core version of their choice. For example, the below steps will generate & install the ``cisco-ios-xr 6.3.1`` bundle compatible with ``ydk core 0.6.2`` (assuming you have already installed the `system dependencies <https://github.com/CiscoDevNet/ydk-py#system-requirements>`_): | ||
|
||
1) Install libydk (taking CentOS as example) |
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.
FAQ looks great. This section could be improved if you linked to the README containing instructions to install ydk on all the systems other than CentOS or similar documentation.
https://github.com/CiscoDevNet/ydk-gen/tree/master/sdk/cpp#quick-install
@@ -66,22 +64,15 @@ CodecService::encode(CodecServiceProvider & provider, Entity & entity, bool pret | |||
XmlSubtreeCodec codec{}; | |||
return codec.encode(entity, root_schema); | |||
} | |||
try |
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.
Is there a reason for eliminating the try catch around this code?
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.
This hid the real issue from the user. User can have try-catch in their app
@@ -19,12 +19,12 @@ set(samples bgp_create | |||
bgp_xr_read | |||
bgp_xr_opendaylight | |||
bgp_restconf | |||
xe_native_read) |
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.
comment out instead
Thanks for your review @ylil93 ! |