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

Allow setting of the client interface access modifier based on the model access modifier #4820

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pbolduc
Copy link

@pbolduc pbolduc commented Mar 17, 2024

This PR allows changing the client interface access modifier. This PR is to address #1363.

Changes:

  • When run from the command line, the model type TypeAccessModifier is used. When TypeAccessModifier is set in the OpenApiToCSharpClientCommand, it sets the settings ClientInterfaceAccessModifier to the same value. This is required because TypeAccessModifier is found in the Newtonsoft library and not in CSharpClientGeneratorSettings.
  • CSharpClientGeneratorSettings defaults ClientInterfaceAccessModifier to public
  • Updates the C# Client.Interface.liquid to use the new ClientInterfaceAccessModifier setting
  • Add unit tests to ensure changing the access modifier is correctly changed in the generated C#.

This PR does not address valid combination of setting the generated C# class and model to public, but the interface internal. This would require additional work to expose additional arguments to the command line tool and changes to the UI tools. Someone would need to justify the use case that make this additional combination valid and the effort to implement.

Valid in the table means it is valid C# and will compile. This PR really addresses the last item in the table.

valid class model interface Handled?
Yes public public public Yes - worked this way before
Yes public public internal No - use case?
No public internal public No - not valid C#
No public internal internal No - not valid C#
Yes internal public public Yes - worked this way before
Yes internal public internal No - edge case?
No internal internal public No - not valid C#
Yes internal internal internal Yes - This PR fixes this specific combination

@pbolduc
Copy link
Author

pbolduc commented Mar 17, 2024

The failing unit tests pass locally. Is this something that I need to investigate or could it be related to the test runner setup?

image

@pbolduc pbolduc changed the title All setting of the client access modifier. Allow setting of the client interface access modifier based on the model access modifier Mar 17, 2024
@Numpsy
Copy link
Contributor

Numpsy commented Mar 17, 2024

The failing unit tests pass locally. Is this something that I need to investigate or could it be related to the test runner setup?

Looks like the same failure I got in #4785 - I wondered if the test timeout I mentioned at #4785 (comment) simply isn't long enough for the macOS CI runner, though I don't have a Mac to test it locally with so I'm just guessing

@jmevel
Copy link

jmevel commented Mar 18, 2024

Awesome, thanks for your work @pbolduc !

@pbolduc
Copy link
Author

pbolduc commented Apr 5, 2024

Are there any issues with this PR? Our team is prevented using the MS Build task to auto generate our C# client because our models are internal. I have to keep reminding my team, they have to manually generate client using the UI and after generating the C# client, they need to edit the generated interface from public to internal.

Please advise if you would like changes by providing feedback on this PR. I have tried to ensure this PR does not introduce any regressions and only makes minimal changes to allow types to be generated as internal to the assembly.

@jmevel
Copy link

jmevel commented Jul 29, 2024

MacOS build failures should be fixed by #4896

#1363 (comment)

Now that #4896 has been merged, @pbolduc could you please re-run the Macos build?
Latest run is from March 17th.

Hoping this PR could be merged soon.

Thanks

@pbolduc
Copy link
Author

pbolduc commented Jul 29, 2024

MacOS build failures should be fixed by #4896

#1363 (comment)

Now that #4896 has been merged, @pbolduc could you please re-run the Macos build? Latest run is from March 17th.

Hoping this PR could be merged soon.

Thanks

I am not a maintainer on this Repo. I am not clear on how I would "re-run the Macos build". Usually a maintainer needs to re-run the github actions.

@jmevel
Copy link

jmevel commented Aug 2, 2024

MacOS build failures should be fixed by #4896

#1363 (comment)
Now that #4896 has been merged, @pbolduc could you please re-run the Macos build? Latest run is from March 17th.
Hoping this PR could be merged soon.
Thanks

I am not a maintainer on this Repo. I am not clear on how I would "re-run the Macos build". Usually a maintainer needs to re-run the github actions.

Ok thanks for your answer. I thought maybe the creator of the PR could manually re-run the build without having to push a new commit.

I guess we just have to wait for a maintainer then.

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