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: Moved InternalsVisibleTo to .csproj. #19

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

HavenDV
Copy link
Contributor

@HavenDV HavenDV commented Jun 7, 2024

No description provided.

@weshaggard
Copy link
Collaborator

weshaggard commented Jun 7, 2024

@HavenDV is there an advantage to doing this in the csproj over the cs file? IIRC the csproj just generates a file on your behalf which to me doesn't really add any value and in fact causes more temp files to be created.

@HavenDV
Copy link
Contributor Author

HavenDV commented Jun 8, 2024

No big difference, except that we will get rid of this file and all project settings will be stored in one place
I see the benefit in that lately you almost never expect an AssemblyInfo file to be present in a project, because now it is almost always generated, and it is better to put it in the project file.
I don't think generation has even a small impact on build time, besides it is generated for other attributes anyway

Feel free to just decline this, it's just a suggestion

@Ste1io
Copy link

Ste1io commented Jun 9, 2024

+1 for this PR, as it is the more modern approach.

src/OpenAI.csproj Outdated Show resolved Hide resolved
src/OpenAI.csproj Outdated Show resolved Hide resolved
@weshaggard
Copy link
Collaborator

Thanks for the contribution and I buy the argument that this file is getting generated for other attributes already so there isn't any real reason to not add this one. I made one suggestion which makes separates the public key into its own property.

@weshaggard weshaggard merged commit e6fe89c into openai:main Jun 10, 2024
1 check passed
@HavenDV HavenDV deleted the internals-visible-to branch June 11, 2024 08:36
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