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

Refactor ClientGenerator class to use private fields #280

Conversation

Minister944
Copy link
Contributor

@Minister944 Minister944 commented Feb 28, 2024

ClientGenerator:

  • Changed query to _query.
  • Changed variables to _variables.
  • Changed response to _response.
  • Changed data to _data.

Fixes #277

@rafalp
Copy link
Contributor

rafalp commented Feb 28, 2024

Rebase on main and add changelog entry.

@Minister944 Minister944 force-pushed the 277-field-arg-named-query-is-shadowed-by-query-string-in-generated-client-methods branch from 42769ad to 5f6d40d Compare February 28, 2024 14:03
Copy link
Contributor

@rafalp rafalp left a comment

Choose a reason for hiding this comment

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

Looking at the diff this change introduced, I am wondering how much extra work would it be to instead detect when one of args is query and only then use _query. Likewise do same for _variables, _result and _data. 🤔

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Minister944
Copy link
Contributor Author

Looking at the diff this change introduced, I am wondering how much extra work would it be to instead detect when one of args is query and only then use _query. Likewise do same for _variables, _result and _data. 🤔

we would have to pass arguments or set flags to code-generating functions just to determine whether to add _
this does not look complicated, but makes the code less readable and increases the level of difficulty of debugging. what would be the purpose of this?

@leandrolerena
Copy link

I can confirm that the fix solves the issue with variable shadowing/overwriting, the client works as expected.

@rafalp
Copy link
Contributor

rafalp commented Feb 29, 2024

@Minister944

what would be the purpose of this?

To don't hit people with pull requests that change 9k places when they update their codegen version.

Lets see how much complexity this change would actually introduce.

@Minister944
Copy link
Contributor Author

we solved it in a different way #281
I am closing this pr

@Minister944 Minister944 closed this Mar 4, 2024
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.

Field arg named query is shadowed by query string in generated client methods
3 participants