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

Avoid using keywords for arguments in TestHelpers #1084

Merged
merged 4 commits into from Mar 1, 2017

Conversation

Projects
None yet
2 participants
@tchssk
Copy link
Member

tchssk commented Feb 28, 2017

Fix #1083.

@tchssk tchssk force-pushed the tchssk:avoid-keyword branch from 9bf616c to c9e463d Feb 28, 2017

@tchssk tchssk force-pushed the tchssk:avoid-keyword branch from c9e463d to 8a78406 Feb 28, 2017

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented Mar 1, 2017

It might be better not to use local variables in the test helper functions.

@raphael
Copy link
Member

raphael left a comment

Thank you, this is a lot of work! I think we should go ahead and merge this once you have done the changes suggested in the code review. I agree that there may be a better approach for the generic case (maybe local variables should all have __ prefix or something like that).

@@ -218,6 +222,36 @@ func (g *Generator) createTestMethod(resource *design.ResourceDefinition, action
}
}

var arguments map[string]bool

This comment has been minimized.

@raphael

raphael Mar 1, 2017

Member

Please make this a field of the Generator struct instead of a global variable. Maybe reservedNames is a better name for it?

funcs := template.FuncMap{"isSlice": isSlice}
funcs := template.FuncMap{
"isSlice": isSlice,
"initArguments": initArguments,

This comment has been minimized.

@raphael

raphael Mar 1, 2017

Member

Does initArguments need to be called when the template is rendered? It could be instead called by the code prior to rendering the template which would be better.

funcs := template.FuncMap{
"isSlice": isSlice,
"initArguments": initArguments,
"tempvar": tempvar,

This comment has been minimized.

@raphael

raphael Mar 1, 2017

Member

Maybe escape is a better name for this function than tempvar?

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented Mar 1, 2017

Thank you for your great review.

I added a new commit fixing the matters.

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented Mar 1, 2017

Now local variables use _ as suffix. Should we use prefix instead of suffix?

@raphael
Copy link
Member

raphael left a comment

Thank you for applying the changes! one last suggestion in the code review.

"initArguments": initArguments,
"tempvar": tempvar,
"isSlice": isSlice,
"escape": escape,

This comment has been minimized.

@raphael

raphael Mar 1, 2017

Member

escape should probably be a method on Generator this way you won't have to pass $test everywhere in the template (so funcs would be initialized with "escape": g.escape).

This comment has been minimized.

@tchssk

tchssk Mar 1, 2017

Member

You mean Generator has escape as a method and reservedNames as a field, right?

In that way, Generator will have all reserved names in the service and use them for all test helpers. reservedNames also affects the functions which actually does not have reserved names. Is it OK?

This comment has been minimized.

@tchssk

tchssk Mar 1, 2017

Member

The way that TestMethod has escape as a method might be better. In this way, we won't have to pass $test everywhere in the template.

This comment has been minimized.

@raphael

raphael Mar 1, 2017

Member

Yes sorry you're correct. reservedNames and escape should be scoped to TestMethod - not Generator.

This comment has been minimized.

@tchssk

tchssk Mar 1, 2017

Member

Thank you! I added the changes as two commits.

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Mar 1, 2017

Using a suffix is fine too.

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Mar 1, 2017

Thank you!

@raphael raphael merged commit 41ba6d4 into goadesign:master Mar 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

raphael added a commit that referenced this pull request Mar 1, 2017

Avoid using keywords for arguments in TestHelpers (#1084)
* Avoid using variables name same with arguments

in TestHelpers.

* Add reservedNames in TestMethod

* Add Escape() to TestMethod

* Use prefix instead of suffix in Escape()

raphael added a commit that referenced this pull request Mar 1, 2017

Avoid using keywords for arguments in TestHelpers (#1084) (#1088)
* Avoid using variables name same with arguments

in TestHelpers.

* Add reservedNames in TestMethod

* Add Escape() to TestMethod

* Use prefix instead of suffix in Escape()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment