bugfix: local variable 'data' colliding with argument name

[zholti]

This PR makes THIS ISSUE a lot less likely to occur.
The local name "data" in generated functions was colliding with arguments called "data".

Note that no explicit name collision check was added, but "data" inside the function was just renamed to "data_" to make it less likely to collide with arguments.

I have:

• Written a clear PR title and description (above)
• Signed the Khan Academy CLA
• Added tests covering my changes, if applicable
• Included a link to the issue fixed, if applicable
• Included documentation, for new features
• Added an entry to the changelog

[csilvers]

@benjaminjkraft should weigh in too, bit it seems like a good idea to me! Can you add the underscore to all vars, not justdata?

[benjaminjkraft]

Thanks -- this seems like a fine partial solution. Eventually we should probably do some fancy name-mangling but data is probably the biggest issue.

Could you add a changelog entry, as well as the missing snapshot I mention below?

[zholti]

@benjaminjkraft should weigh in too, bit it seems like a good idea to me! Can you add the underscore to all vars, not justdata?

Did that for the local "err" as well. The collision with query/mutation args affects only this template as far as I can see. Which other vars in which templates can be affected?

[benjaminjkraft]

As far as I can tell this is the only template that uses user-defined variable names. To be complete we should do the same for req and resp. If you get a chance to do that that would be great. (In theory we should also do ctx and client, but since those are user-visible, I'd prefer if we can change them only if needed. And they're probably less likely names to conflict with, since context is a Go thing and how would you pass a client over GraphQL.)

[csilvers]

I could see you passing in a var called client to say which client to use. But I agree we don't have to handle it now if it's user facing

[zholti]

I could see you passing in a var called client to say which client to use. But I agree we don't have to handle it now if it's user facing

Passing a client is already there, no? Depending on Config.ClientGetter, either the one passed as arg or the local one will be constructed.

But you do have a point - no matter if the client is passed or constructed, an arg called "client" will collide with it in both cases.

[zholti]

As far as I can tell this is the only template that uses user-defined variable names. To be complete we should do the same for req and resp. If you get a chance to do that that would be great. (In theory we should also do ctx and client, but since those are user-visible, I'd prefer if we can change them only if needed. And they're probably less likely names to conflict with, since context is a Go thing and how would you pass a client over GraphQL.)

In my latest commit, I renamed client to client_, since I can imagine that there might be schemas with fields taking such an arg. Should I also do the same treatment on ctx? This is less likely to collide with argument names, but still...

[benjaminjkraft]

If we're gonna do client_, we may as well do the same for ctx_!