Added support for multiple entities for Go #768

ygorelik · 2018-04-24T17:19:52Z

Added support for multiple entities for Go

codecov · 2018-04-24T18:47:23Z

Codecov Report

Merging #768 into master will increase coverage by 0.06%.
The diff coverage is 88.06%.

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
+ Coverage    80.7%   80.77%   +0.06%     
==========================================
  Files         171      171              
  Lines       16549    16756     +207     
  Branches     1493     1495       +2     
==========================================
+ Hits        13356    13534     +178     
- Misses       2828     2847      +19     
- Partials      365      375      +10

Impacted Files	Coverage Δ
sdk/go/core/ydk/entity_lookup.go	`100% <100%> (+28.57%)`	⬆️
ydkgen/printer/go/class_printer.py	`95.87% <100%> (ø)`	⬆️
sdk/cpp/core/src/ydk.cpp	`25.65% <16.66%> (ø)`	⬆️
sdk/go/core/ydk/services/services.go	`63.75% <25%> (-0.85%)`	⬇️
sdk/cpp/core/src/path/netconf_session.cpp	`86.28% <50%> (+0.04%)`	⬆️
sdk/python/core/ydk/entity_utils/entity_utils.py	`74.48% <60%> (-7.24%)`	⬇️
sdk/go/core/ydk/path/path.go	`89.82% <90.81%> (-1.2%)`	⬇️
sdk/go/core/ydk/types/types.go	`79.14% <91.25%> (+10.49%)`	⬆️
... and 2 more

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 1b81b90...f9b017a. Read the comment docs.

ylil93

Thanks for the update Yan!

ylil93 · 2018-04-24T21:59:10Z

sdk/cpp/tests/test_netconf_service.cpp

@@ -328,30 +328,3 @@ TEST_CASE("ietf_get_rpc")
        cout << "Exception while executing RPC: " << ex.what() << endl;
    }
 }
-
-TEST_CASE("get_config_openconfig_interfaces_and_bgp")


Is there a reason for deleting this test case?

It exposes issue 727. I will put it back after fixing the issue.

I see. It's probably better to comment it out with that explanation rather than deleting everything.

See this example:
https://github.com/ygorelik/ydk-gen/blob/715bed51b22bf3cb9e1e8d69aee23f9ee2d9544e/sdk/cpp/tests/test_netconf_service.cpp#L40

ylil93 · 2018-04-24T22:05:55Z

sdk/go/core/docsgen/api/types/ydk_types.rst

@@ -268,44 +268,172 @@ These are how YANG types are represented in Go.

    An interface type that represents a basic container in YANG

-    .. function:: GetEntityData()
+    .. function:: GetEntityData() Entity


This function doesn't return Entity; it returns *CommonEntityData. The line should be
.. function:: GetEntityData() *CommonEntityData

ylil93 · 2018-04-24T22:08:30Z

sdk/go/core/docsgen/api/types/ydk_types.rst

-    .. function:: GetEntityData()
+    .. function:: GetEntityData() Entity
+
+        The :ref:`Entity <types-entity>` interface function


This is confusing. Instead say something like
Implements :ref:`Entity ` interface

I did not see the change in your latest push - bumping this comment so it doesn't get buried

ylil93 · 2018-04-24T22:11:48Z

sdk/go/core/docsgen/api/types/ydk_types.rst

+
+    .. function:: GetEntityData() *CommonEntityData
+
+        Implements Entity interface.


I think it's better to include the link to to the Entity interface:

Implements :ref:`Entity ` interface

I did not see the change in your latest push - bumping this comment so it doesn't get buried

It is in my fork: ygorelik@d227bd9

ygorelik · 2018-04-26T14:58:08Z

sdk/go/core/tests/service_crud_test.go

@@ -3,7 +3,8 @@ package test
 import (
 	"fmt"
 	ysanity "github.com/CiscoDevNet/ydk-go/ydk/models/ydktest/sanity"
-	// "github.com/CiscoDevNet/ydk-go/ydk/models/ietf/interfaces"


ylil93 · 2018-04-24T22:19:49Z

sdk/go/core/ydk/path/path.go

 	}
-
+	


Extra indentation should be deleted

Deleted. Will check-in with next commit due to code overlapping with project in work.

ylil93 · 2018-04-24T22:29:25Z

sdk/go/core/ydk/types/types.go

+	Parent 					Entity
+}
+
+func (e *CommonEntityData) GetPath() string {


Is this method necessary? For the purpose of extracting the segment path from an Entity struct, this method exists
https://github.com/ygorelik/ydk-gen/blob/715bed51b22bf3cb9e1e8d69aee23f9ee2d9544e/sdk/go/core/ydk/types/types.go#L316

Deleted. Will check-in with next commit due to code overlapping with project in work.

ghost · 2018-04-25T16:20:58Z

sdk/cpp/core/src/path/netconf_session.cpp

@@ -489,8 +489,9 @@ static string get_filter_payload(path::Rpc & ydk_rpc)
 {
    auto entity = ydk_rpc.get_input_node().find("filter");
    if(entity.empty()){
-        YLOG_ERROR("Failed to get entity node.");
-        throw(YInvalidArgumentError{"Failed to get entity node"});
+        YLOG_WARN("Failed to get filter node for RPC; it then will be skipped.");


Why is this changed to warning?

It is not an error when filter is not present. In this case the whole device configuration will be fetched.

ghost · 2018-04-25T16:23:21Z

sdk/cpp/core/src/ydk.cpp

        case JSON:
            return ydk::EncodingFormat::JSON;
+        case XML:
+        default:


default cases should be avoided. Suggest removing this or adding cases for all the members of the enum

The compiler logged warning message pointing that 'default' case is missing. I was taught always include 'default' case. If not included the 'switch' must be re-factored to if-else statements.

ghost · 2018-04-25T16:24:02Z

sdk/cpp/core/src/ydk.cpp

        case Restconf:
            return ydk::Protocol::restconf;
+        case Netconf:
+        default:


Same as above. Better to avoid default switch cases

The compiler logged warning message pointing that 'default' case is missing. I was taught always include 'default' case. If not included the 'switch' must be re-factored to if-else statements.

ghost · 2018-04-25T16:24:52Z

sdk/go/README.md

@@ -64,6 +64,16 @@ $ sudo yum install https://devhub.cisco.com/artifactory/rpm-ydk/0.7.1/libydk-0.7

 ```

+Golang


Can this be made to a heading? Something like

### Golang version

ghost · 2018-04-25T16:26:27Z

sdk/go/core/docsgen/conf.py

@@ -36,6 +32,8 @@

 sys.path.append(os.path.abspath(ydk_root+'/sphinxexts'))
 extensions = [ 'godomain' ]
+print(extensions, sys.path)


Can this print statement be removed?

It is leftover from debug session. Removed.

Yan Gorelik and others added 19 commits April 11, 2018 11:57

Introduced EntityCollection type

eb26353

Merge remote-tracking branch 'upstream/master'

1c4a191

Fixed CiscoDevNet#751

dd01ec2

Merge remote-tracking branch 'upstream/master'

827f5f0

Added support for multiple entities in Go

7f1de26

Fixed Codacy issues

1915f1c

Removed OC test

e9c0468

Merge remote-tracking branch 'upstream/master'

da4dd5b

Bumped required Go version to 1.9

8da2aa3

Bumped required Go version to 1.9

e991527

Bumped required Go version to 1.9

39a2708

Bumped required Go version to 1.9

4d523c4

Bumped required Go version to 1.9

9915bb7

Bumped required Go version to 1.9

990865d

Bumped required Go version to 1.9

ea46b5d

Bumped required Go version to 1.9

f1e6b30

Merge branch 'master' into master

185402a

Bumped required Go version to 1.9

f35df3a

Merge remote-tracking branch 'origin/master'

715bed5

ylil93 suggested changes Apr 24, 2018

View reviewed changes

Merge branch 'master' into master

3d21dbb

ghost suggested changes Apr 25, 2018

View reviewed changes

Yan Gorelik and others added 4 commits April 26, 2018 08:29

Addressed comments to pull request

d227bd9

Merge remote-tracking branch 'origin/master'

9a7bb47

Implemented OrderedMap functionality on YChild and YLeaf maps

796ab8a

Merge branch 'master' into master

f9b017a

ylil93 approved these changes Apr 27, 2018

View reviewed changes

ghost approved these changes Apr 27, 2018

View reviewed changes

ghost merged commit 6a6827c into CiscoDevNet:master Apr 27, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for multiple entities for Go #768

Added support for multiple entities for Go #768

ygorelik commented Apr 24, 2018

codecov bot commented Apr 24, 2018 •

edited

Loading

ylil93 left a comment

ylil93 Apr 24, 2018

ygorelik Apr 26, 2018

ylil93 Apr 26, 2018

ylil93 Apr 24, 2018

ygorelik Apr 26, 2018

ylil93 Apr 24, 2018 •

edited

Loading

ygorelik Apr 26, 2018

ylil93 Apr 26, 2018

ylil93 Apr 24, 2018

ygorelik Apr 26, 2018

ylil93 Apr 26, 2018

ygorelik Apr 26, 2018

This comment was marked as resolved.

ygorelik Apr 26, 2018

ylil93 Apr 24, 2018

ygorelik Apr 26, 2018 •

edited

Loading

ylil93 Apr 24, 2018

ygorelik Apr 26, 2018

ghost Apr 25, 2018

ygorelik Apr 26, 2018

ghost Apr 25, 2018

ygorelik Apr 26, 2018

ghost Apr 25, 2018

ygorelik Apr 26, 2018

ghost Apr 25, 2018

ygorelik Apr 26, 2018

ghost Apr 25, 2018

ygorelik Apr 26, 2018


		.. function:: GetEntityData() *CommonEntityData

		Implements Entity interface.

		@@ -64,6 +64,16 @@ $ sudo yum install https://devhub.cisco.com/artifactory/rpm-ydk/0.7.1/libydk-0.7

		```

		Golang

Added support for multiple entities for Go #768

Added support for multiple entities for Go #768

Conversation

ygorelik commented Apr 24, 2018

codecov bot commented Apr 24, 2018 • edited Loading

Codecov Report

ylil93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylil93 Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygorelik Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 24, 2018 •

edited

Loading

ylil93 Apr 24, 2018 •

edited

Loading

ygorelik Apr 26, 2018 •

edited

Loading