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

initial commit for leaf setters #281

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

adibrastegarnia
Copy link

@adibrastegarnia adibrastegarnia commented Apr 7, 2019

Initial commit for adding leaf setters

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@adibrastegarnia
Copy link
Author

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

memo Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers
* It's possible we don't have your GitHub username or you're using a different email address on your commit. Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).
Corporate signers
* Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to [go/cla#troubleshoot](http://go/cla#troubleshoot) ([Public version](https://opensource.google.com/docs/cla/#troubleshoot)).

* The email used to register you as an authorized contributor must be the email used for the Git commit. Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).

* The email used to register you as an authorized contributor must also be [attached to your GitHub account](https://github.com/settings/emails).

information_source Googlers: Go here for more info.
I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 91.014% when pulling fa4bf01 on adibrastegarnia:add-leaf-setters into 0c7dd99 on openconfig:master.

@coveralls
Copy link

coveralls commented Apr 7, 2019

Coverage Status

Coverage decreased (-0.2%) to 91.004% when pulling da31991 on adibrastegarnia:add-leaf-setters into 0c7dd99 on openconfig:master.

@adibrastegarnia
Copy link
Author

@robshakir
I started to add leaf setters to the list of helper functions. Would you please take a look so far and give me your comments?

@adibrastegarnia
Copy link
Author

adibrastegarnia commented Apr 7, 2019

@robshakir
In addition, I am getting a compile error on my end but I don't know how I pass all of the checks:
generator/generator.go:255:23: unknown field 'GenerateLeafSetters' in struct literal of type ygen.GoOpts

I shouldn't get the error because I already defined it but I don't know why it happens

P.S. Resolved

@adibrastegarnia
Copy link
Author

adibrastegarnia commented Apr 8, 2019

@robshakir
I assume that setter function does not return anything if I need to return error or ok or a value please let me know. I tested the patch and it looks like that is working but that would be great if you check it as well.

Copy link
Contributor

@robshakir robshakir 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 contribution, there are some technical issues with this change that I've pointed out in the review.

Additionally, there's no unit testing for this change -- please add tests that both check the generated sets of methods are correct for the individual code, as well as an integration test that does this across a real input module (integration test with goyang). You can see some examples of these in TestSimpleStructs in codegen_test.go.

Also, please let's add a test to schema_tests in this package that tests more than just string fields.

Happy to discuss this design more if there's more info required as to the implementation here.

Thanks!
r.

goLeafSetterTemplate = `
// Set{{ .Name }} sets the value of the leaf {{ .Name }} from the {{ .Receiver }}
// struct.
func (t *{{ .Receiver }}) Set{{ .Name }}(val string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that you can just take a string as an argument, there are different types of fields that we have within the generated structs. Given that this is the case, we need to handle different inputs here. Determining the type that is needed should likely be from the input type that is in the generatedLeafSetter that you created. There are some other details that you'll need though, such as the name of the generated union, such that you can call To_XXX for these types.

Copy link
Author

Choose a reason for hiding this comment

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

@robshakir
Thanks for your comments. That is a good point. I will start working on it but before that what types you would suggest to support? I will let you know if I have more questions.

Copy link
Author

@adibrastegarnia adibrastegarnia Apr 14, 2019

Choose a reason for hiding this comment

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

I think I figured that out.

Copy link
Author

@adibrastegarnia adibrastegarnia Apr 14, 2019

Choose a reason for hiding this comment

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

@robshakir

May I ask you how do you generate the expected output for the testcases? It says the following but it is not clear for me:
This package was generated by codegen-tests
using the following YANG input files: .....

Copy link
Author

@adibrastegarnia adibrastegarnia Apr 15, 2019

Choose a reason for hiding this comment

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

@robshakir

Please take a look at this code and I think it handles any type. Please let me know what you think. I also have a test case ready but I am not sure how do you generate the expected output in advance.

// goLeafSetterTemplate defines a template for a function that, for a
	// particular leaf, generates a setter method.
	goLeafSetterTemplate = `
// Set{{ .Name }} sets the value of the leaf {{ .Name }} from the {{ .Receiver }}
// struct. 
func (t *{{ .Receiver }}) Set{{ .Name }}(val interface{}) {
	if t == nil || val == nil {
		return
	}
	t.{{ .Name }} = ygot.ToPtr(val).(*{{ .Type }})
}

`

if t == nil || val == "" {
return
}
t.{{ .Name }} = ygot.String(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above re: whether you can use string.

This likely highlights a test coverage gap -- we should add tests to schema_tests which give an integration test against a YANG model that has >1 different field type.

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.

4 participants