Bus should be a property not a field #432
Conversation
Bus should be a property not a field Compliance with analyser rule: https://docs.microsoft.com/en-gb/visualstudio/code-quality/ca1051-do-not-declare-visible-instance-fields This is shallow fix in order to get the analysers passing, pending better design
That's not wrong. |
but arguably no worse and the tests pass This PR is aimed at the analyser warning in JustSaying, not the tests and did I mention that the tests pass now?
| @@ -28,7 +28,7 @@ public class JustSayingFluently : ISubscriberIntoQueue, IHaveFulfilledSubscripti | |||
| private readonly ILogger _log; | |||
| private readonly IVerifyAmazonQueues _amazonQueueCreator; | |||
| private readonly IAwsClientFactoryProxy _awsClientFactoryProxy; | |||
| protected IAmJustSaying Bus { get; } | |||
| protected IAmJustSaying Bus { get; private set; } | |||
| private SqsReadConfiguration _subscriptionConfig = new SqsReadConfiguration(SubscriptionType.ToTopic); | |||
There was a problem hiding this comment.
Could we get away with internal and use InternalsVisibleTo?
There was a problem hiding this comment.
If the test isn't super important and just belt-and-braces, we should remove it. It's a self-documented cludge, so if it's not adding much value we should just 🗑 it if it's causing us trouble here.
There was a problem hiding this comment.
The test looks like it's a base class for a lot of things :(
There was a problem hiding this comment.
This is the theme in general with a lot of the tests
There was a problem hiding this comment.
I meant for the whole property, not just the setter.
There was a problem hiding this comment.
Well TIL. https://stackoverflow.com/a/1063920/2225808
You could have a method that sets it, and is internal. 🤔 🤢
There was a problem hiding this comment.
Yeah, protected internal for the property is what we want
|
Turns out there are more of those reflection setters 😆 |
|
That's better. |
Summarise the changes this Pull Request makes.
Bus should be a get-only property not a read-only field
Compliance with analyser rule: https://docs.microsoft.com/en-gb/visualstudio/code-quality/ca1051-do-not-declare-visible-instance-fields
This is a quick, shallow fix in order to get the analysers passing, pending a better design.
Please include a reference to a GitHub issue if appropriate.
#401