Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[.NET] Added .NET 8, NRT and more #253
[.NET] Added .NET 8, NRT and more #253
Changes from 3 commits
d2b13ac
9b9679a
99873a7
6f9d7aa
dd83471
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template was generating "record-like" classes with getters only; in a sense providing an 'immutable' data structure. This PR is adopting the use of records directly, making a stronger statement about immutability.
During the constructor, we previously copyied the contents of enumerable items by calling 'new List(argument) for those properties that were enumerable. This disconnected the Message instance from its source (if one list or the other got changed, the other would not).
This proposed template simply assigns the input argument to the generated property.
@gasparnagy How careful do we need to be about immutability and/or effects of change here?
We could use an ImmutableList here to be explicit about intent and strengthen the behavior, go back to previous behavior of simply copying the list content, or accept this behavior of accepting the List argument as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImmutableArray is desirable here, but i will require 1 extra dependency for .NET Standard tfm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think regardless of what structure we use to store the "immutable" list, we should go back to the previous model of "copying" the items from the input structure, so the simple assignment is not enough.
As this is package is not really used yet, maybe we should be more future proof and go for the ImmutableList accepting the fact that this way we have to add a dependency to
System.Collections.Immutable
for the .NET Standard 2.0 target (in .NET 8 it is anyway included). So I would say, let's give it a try. If we see problems with this dependendency, we can still roll back.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx. As far as I see now it takes
ImmutableArray
as an input, so it will be the responsibility of the caller to ensure the immutability. I think this is good, but I wonder if we should generate an additional convenience overloads for the ctors that just take anIEnumerable<T>
and build theImmutableArray
from it.@clrudolphi Would creating an
ImmutableArray
for calling the ctors cause any issues for you?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the constructor signature to accept the IEnumerable as a parameter, and have it create the ImutableArray property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that we need to make a copy inside of constructor without any visible reason.
Maybe you have some code examples why it makes a problem and I can propose a some solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's clarify what we mean. Likewise, I do not think it necessary to create copies of the items within the source collections.
The design principle under consideration here is to provide a construction signature that is easy to use. The MS API Design Guidelines suggest that collection parameters be typed as interfaces and of a commonly used type, such as IEnumerable or IReadOnlyCollection.
As a convenience to users of the constructor, we can build an instance of an ImmutableArray from that provided IEnumberable argument and assign that to the property. Doing so doesn't copy the members of the collection (presuming T is a class, not a value type).
Since ImmutableArray is also IEnumerable, within the Messages library we can pass instances of ImmutableArray to constructors of other types (that accept IEnumberable) without needing an overloaded constructor.
If there is a concern about performance or memory overhead of creating a new instance of ImmutableArray out of an input IEnumberable, why not have the constructor test the run-time type of the argument, and if it is already an ImmutableArray, then simply assign it to the property; otherwise new up a new instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, then lets continue to use List copy as we have now