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

GitHub issues rebase #679

Merged
3 commits merged into from
Feb 6, 2018
Merged

GitHub issues rebase #679

3 commits merged into from
Feb 6, 2018

Conversation

ylil93
Copy link
Contributor

@ylil93 ylil93 commented Feb 5, 2018

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks @ylil93. Just had couple of minor suggestions.

@@ -114,11 +114,25 @@ ydk::path::DataNodeImpl::populate_new_schemas_from_path(const std::string& path)
ydk::path::DataNode&
ydk::path::DataNodeImpl::create_datanode(const std::string& path, const std::string& value)
{
std::string v = value;
Copy link

Choose a reason for hiding this comment

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

Suggest moving lines 117-129 into a separate function called something like 'escape_xml_declaration()'. This is so that it is clear what is being done by these lines of code.

@@ -128,6 +128,21 @@ def test_get_schema(self):
xml = self.codec.encode(res, EncodingFormat.XML, False)
self.assertNotEqual( len(xml), 0 )

def test_anyxml(self):
Copy link

Choose a reason for hiding this comment

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

Could you please also add a testcase in test_sanity_codec.py ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did...

@codecov
Copy link

codecov bot commented Feb 5, 2018

Codecov Report

Merging #679 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   74.17%   74.22%   +0.04%     
==========================================
  Files         104      104              
  Lines       10453    10470      +17     
  Branches     1467     1467              
==========================================
+ Hits         7754     7771      +17     
  Misses       2397     2397              
  Partials      302      302
Impacted Files Coverage Δ
sdk/python/core/tests/test_sanity_codec.py 94.87% <100%> (+0.21%) ⬆️
sdk/python/core/tests/test_sanity_path.py 98.76% <100%> (+0.15%) ⬆️

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 e2a86a0...1ec2c5b. Read the comment docs.

@ghost ghost merged commit cfa8cdc into CiscoDevNet:master Feb 6, 2018
@ghost
Copy link

ghost commented Feb 6, 2018

Thanks @ylil93 for this change!

@ylil93 ylil93 deleted the github_issues_rebase branch March 2, 2018 18:42
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