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

C# plugin improvements #4242

Merged
merged 3 commits into from
Jun 18, 2020
Merged

C# plugin improvements #4242

merged 3 commits into from
Jun 18, 2020

Conversation

cklam2
Copy link
Contributor

@cklam2 cklam2 commented Jun 16, 2020

Solves #4214.

Additionally added some more improvements:

  • Nullable value types
  • Initial value support
  • Class and field documentation in summary block
  • Deprecated fields have DeprecatedAttribute
  • Required input fields have JsonRequiredAttribute
  • Generic implementation to compose input object
  • Code in namespace indented and wrapped

There's one incompatibility though: the generated C# method getInputObject is renamed to GetInputObject just to follow the general C# naming convention. If breaking compatibility is a no-go to you, it can be reverted.

@dotansimha
Copy link
Owner

This is great! Thank you @cklam2 !
Any chance you can add some unit tests to make sure we are not breaking these changes in the future?

@cklam2
Copy link
Contributor Author

cklam2 commented Jun 17, 2020

@dotansimha
Yep, already added some unit tests (locally) for the fixes but they are not complete yet. Eg missing tests for existing code. Ok to do another pull request once they are done? Or you want it be included in this one?

@dotansimha
Copy link
Owner

I think if tests are missing for the existing code, we can improve it in a future PR. Let me know when you push the tests for the modified code, and we'll progress on that.

Thank you!

cklam2 added 3 commits June 17, 2020 13:37
Nullable value types
Initial value support
Class and field documentation in summary block
Deprecated fields have DeprecatedAttribute
Required input fields have JsonRequiredAttribute
Generic implementation to compose input object
Code in namespace indented and wrapped
@cklam2
Copy link
Contributor Author

cklam2 commented Jun 17, 2020

Well, glad that I extended the unit tests cause found some regressions in the initial values part and the array composition. Also refactored some of the code, hence all those changes.

Also committed the unit test for the c-sharp plugin. Works locally. Only not sure if I need to commit a jest.config.js or need to change package.json. @dotansimha can you check if the unit test needs some configuration in the project?

Rebased the branch to latest upstream master.

@dotansimha
Copy link
Owner

This is great, thank you so much @cklam2 !

@dotansimha dotansimha merged commit be3a13d into dotansimha:master Jun 18, 2020
@theguild-bot
Copy link
Collaborator

The latest changes of this PR are available as alpha in npm: 1.15.4-alpha-be3a13de.0

Quickly update your package.json by running:

npx match-version @graphql-codegen 1.15.4-alpha-be3a13de.0

@cklam2 cklam2 deleted the c-sharp-improvements branch June 20, 2020 22:53
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.

None yet

3 participants