Skip to content

Improve documentation, fix error messages#622

Merged
4 commits merged intomasterfrom
unknown repository
Nov 9, 2017
Merged

Improve documentation, fix error messages#622
4 commits merged intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
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.

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
Copy Markdown
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.

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

Choose a reason for hiding this comment

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

Same as above

bgp_xr_read
bgp_xr_opendaylight
bgp_restconf
xe_native_read)
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

XmlSubtreeCodec codec{};
return codec.encode(entity, root_schema);
}
try
Copy link
Copy Markdown
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
Copy Markdown
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

bgp_xr_read
bgp_xr_opendaylight
bgp_restconf
xe_native_read)
Copy link
Copy Markdown
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
Copy Markdown
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.

1 participant