Convert Binding to SLOOP Dto #5237
Conversation
ugras-ergun-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
|
Most of the files are packages changes they're in different commits. Look at the 2nd commit for the real changes. |
|
|
||
| public IEnumerable<T> GetServerConnectionConfiguration<T>() where T : ServerConnectionConfiguration | ||
| { | ||
| if (bindingList == null) { InitBindingList(); } |
There was a problem hiding this comment.
How is this going to handle new connections added?
There was a problem hiding this comment.
I am going to handle that while doing the handle new binding ticket.
There was a problem hiding this comment.
But why make this class stateful in the first place then? You can make it always get the most relevant data, and then add caching later if you need to.
There was a problem hiding this comment.
I was planning to have an add method later and add to list (if the conn does not exist already). I don't want to do IO operation everytime we call this class, I know probably I won't be that much but still.
There was a problem hiding this comment.
Why add that method here and not create a caching wrapper for IBindingRepository?
| return true; | ||
| } | ||
|
|
||
| if (x == null && y == null) { return true; } |
There was a problem hiding this comment.
is null everywhere around
|
|
||
| if (x == null && y == null) { return true; } | ||
|
|
||
| if (x == null ^ y == null) |
There was a problem hiding this comment.
This is intuitively easier to understand, and does the same thing.
| if (x == null ^ y == null) | |
| if (x == null || y == null) |
There was a problem hiding this comment.
but they are not the same I want to XOR them it should be true when they're both null.
| return true; | ||
| } | ||
|
|
||
| if (x == null && y == null) { return true; } |
There was a problem hiding this comment.
The formatting is inconsistent around (single-line if vs. multi-line if)
ugras-ergun-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| { | ||
| return true; | ||
| } | ||
| //XOR it's true only when one of them is true |
There was a problem hiding this comment.
why xor when you check that both are true in the condition above?
There was a problem hiding this comment.
I think having an OR there would be misleading when the intended behaviour is XOR is confusing, even the case when both true are covered by the expression before.
There was a problem hiding this comment.
No, it's not misleading. You don't just forget the condition above when you read code. XOR is so infrequently used that it's actually more confusing to see it.
There was a problem hiding this comment.
I do not agree but it's a so trivial issue that doesn't deserve any more discussion about it.
| return false; | ||
| } | ||
|
|
||
| return x.ServerUri == y.ServerUri; |
There was a problem hiding this comment.
I would use Equals instead just to be safe
There was a problem hiding this comment.
== is already overridden for Uri, so it wouldn't be safer also if the ServerUri of the first Uri were null somehow (It shouldn't be), it would create an exception.
There was a problem hiding this comment.
The fact that == is overriden is an additional knowledge that is not apparent at the first glance. It's more natural to see Equals delegation inside Equals. Also, the uri is never null there. There's a check in the constructor
ugras-ergun-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| } | ||
| } | ||
|
|
||
| return bindingList.OfType<T>(); |
There was a problem hiding this comment.
This doesn't make much sense after you removed caching. I would return the list of ServerConnectionConfiguration and let the caller filter that instead
There was a problem hiding this comment.
caller can still do it if they pass ServerConnectionConfiguration as T.
There was a problem hiding this comment.
But if they don't, they will do IO twice. There's no need for this generic now.
|
|
||
| public int GetHashCode(BoundSonarQubeProject obj) | ||
| { | ||
| return obj.ServerUri.GetHashCode(); |
There was a problem hiding this comment.
You check for null in Equals, but not here
There was a problem hiding this comment.
We don't need to here it's the expected behaviour.
| namespace SonarLint.VisualStudio.ConnectedMode.UnitTests.Binding | ||
| { | ||
| [TestClass] | ||
| public class ServerConnectionConfigurationProviderTests |
There was a problem hiding this comment.
Missing MEF tests
ugras-ergun-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
|
georgii-borovinskikh-sonarsource
merged commit e593fa9
into
feature/sloop-rule-meta-data
Fixes #5229