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
Enable vulnerability checking for packages.config projects during commandline restore #5639
Enable vulnerability checking for packages.config projects during commandline restore #5639
Changes from all commits
35a7083
5c958d8
4d95b9b
1c9f719
ab1a9a2
1f929b1
fc2b65b
04b44de
9f085f2
b062e19
10f0db8
854d71e
81a159c
9ce4ae5
5dbc2ed
41f0379
437c307
0b53e4c
6fd9d75
7b476bf
003708f
9d62ce5
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.
Considering this type is a
record
, and therefore astruct
, I think it's important to make all these propertiesget; init;
, rather thanget; set;
.The problem with
get; set;
in a struct, is if one is passed in as a method parameter (but not as aref
, changing the value of properties will cause the values to be lost when the method returns. So, it's significant risk of bugs if people use it wrong.Making it only possible to set at initialization time reduces the risk that it gets set wrong.
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.
These are not always known/available in every audit situation.
I have tests ensuring things get set in the expected scenarios. Obviously that doesn't guard against future additions.
I can have a bunch of internal constructors I guess if you'd prefer that.
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.
init
does not meanrequired
, it just means that the type is read only, and needs to have a new "instance" created to change a value. Using thewith
expression makes this very easy. Therefore multiple constructors are not needed, unless you want to validate that when "property 1" is set "property 2" must be as well.And when I reviewed this PR last week, I didn't see any examples of the type having any values changed after initialization. At least in the product code, I only saw properties being set on init.
This is the point of my suggestion. It does prevent future incorrect usage that might otherwise go unnoticed.