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 documentation, fix error messages #622

Merged
4 commits merged into from Nov 9, 2017
Merged

Improve documentation, fix error messages #622

4 commits merged into from Nov 9, 2017

Conversation

ghost
Copy link

@ghost 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
Copy link

codecov bot commented Nov 9, 2017

Codecov Report

Merging #622 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
generate.py 50.21% <0%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48217e8...a19b735. Read the comment docs.

Copy link
Contributor

@ylil93 ylil93 left a 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");
Copy link
Contributor

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");
Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment out instead

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


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)
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Author

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment out instead

@ghost
Copy link
Author

ghost commented Nov 9, 2017

Thanks for your review @ylil93 !

@ghost ghost merged commit e08506a into CiscoDevNet:master Nov 9, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant