Readonly response models #658
Conversation
|
|
||
| /// <summary> | ||
| /// The <seealso cref="User"/> that created the deployment. | ||
| /// </summary> | ||
| public User Creator { get; set; } | ||
| public User Creator { get; protected set; } | ||
|
|
||
| /// <summary> | ||
| /// JSON payload with extra information about the deployment. | ||
| /// </summary> | ||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")] |
There was a problem hiding this comment.
Can these be removed now? There are about 12 of them...
There was a problem hiding this comment.
Thinking about it, shouldn't collection properties be initialized in the constructors and have private setters? I assume SimpleJson supports it since it supports protected setters?
There was a problem hiding this comment.
We shouldn't initialize them in the ctors for these types because they'll just get replaced when SimpleJson deserializes the json into an object. It uses the private setters to set the collection.
There was a problem hiding this comment.
Just a follow-up. For "request models" what you described is exactly correct. The ctor should instantiate the collection and the setter should be private. I made that change. Let me know if you find any cases I missed.
|
Also, does 1e1039f mean that all |
|
Quite possibly! I'll test it out. |
|
Also, it looks like there were some new model types I missed when I merged master in. |
|
SimpleJson doesn't support readonly dictionaries. And yet another yak appears! |
|
Ok, I created a PR to simple-json. In the meanwhile, I'll just edit our copy directly. |
|
Ok, I think this is ready. Would love some 👀 on it. Thanks for the excellent review so far @khellang. |
It's bothered me that our response models are mutable. They really shouldn't be. I made the properties have a protected setter instead of private to provide flexibility for others who might be deriving from these classes or testing them. Fixes #650
This test tried to serialize a response type. We only DEserialize those.
Looks like we're not keeping the other platform unit test projects up to date with the main one.
I need to submit a PR for this upstream as well.
Also added a test for it.
Found a bug in which we were using Name when we should be using Login.
They are now readonly and implement a generic base class.
haacked
force-pushed
the
haacked/readonly-response-models
branch
from
cb33852 to
60373ea
Compare
| public SignatureResponse Author { get; protected set; } | ||
| public SignatureResponse Committer { get; protected set; } | ||
| public GitReference Tree { get; protected set; } | ||
| public IEnumerable<GitReference> Parents { get; protected set; } |
There was a problem hiding this comment.
IReadOnlyList<GitReference>?
|
Changed test in khellang@619a282 to verify handling of readonly collections, and it works beatifully 😁 Maybe there should be a convention test that verifies that collections are of the right type? Is there a reason for sometimes using |
Good question. I don't see any benefit to using collection over list here. List provides an indexer which can be handy. |
|
🍧 |
❤️❤️❤️❤️❤️ #660 |
For response models.
haacked
force-pushed
the
haacked/readonly-response-models
branch
from
5ec1d51 to
1dd42ec
Compare
|
🍍 |
|
|
This makes our response models readonly. I made the setters
protectedinstead ofprivateto provide flexibility for others such as inheritors, testers, etc.. Also, it gets rid of that warning that the setter is never used. Let me know what you think.Fixes #650