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#] add support for init-only setters on generated classes #1155

Closed
wants to merge 1 commit into from

Conversation

enioluwas
Copy link
Contributor

Closes #1154 by introducing an --init-only-properties switch for the Bond C# compiler.

Mostly just mimicked the implementation for the --readonly-properties switch. This was tricky to add unit tests for, because the init keyword was introduced in C# 9 / .NET 5. Please review and see if it's okay as-is. I considered adding a new unit test project for .NET 5+, but it wouldn't be compatible with VS 2017.

@enioluwas enioluwas force-pushed the master branch 2 times, most recently from 61a637d to 2b6c3f8 Compare June 29, 2022 01:16
Copy link
Member

@chwarr chwarr left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. Can you add a changelog entry? This is a minor version bump for C#.

I forgot that init properties were a C# 9+ and .NET 5+ feature. Sigh. As you've noted, the CI build only uses the .NET Core 3.1 SDK today.

Bond still needs to support .NET Framework, so we can't require .NET 5 or newer. In issue #1143, Deprecating Bond support for EOL platforms and toolchains, @bhardwajs proposed a new minimum set of supported platforms, including updating the Windows CI build to Visual Studio 2019/dotnet. That needs to land before this can be merged. I don't see a way to test this new code without using a newer toolchain to build...

I'm not comfortable merging in code that doesn't have CI test coverage.

I do not have any time in the foreseeable future myself to work on Bond's CI builds. I'm going to need to rely on you or @bhardwajs to contribute the needed build modernization here.

Once that's done, then all the init code can be put behind conditional compilation and only be done on .NET 5+.

@enioluwas
Copy link
Contributor Author

Alright, will hold off on this, and consider helping with #1143

@enioluwas enioluwas marked this pull request as draft December 3, 2022 22:57
@chwarr
Copy link
Member

chwarr commented Mar 24, 2023

It has been more than six months since there have activity on this PR. I am going to close it for now. If you are still interested in working on this, let me know, and I can re-open this. The .NET build changes will still need to come first. Thanks.

@chwarr chwarr closed this Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C#] Support for init properties on generated classes
2 participants