Merge classes into ISolutionBindingRepository #5225
Conversation
| @@ -1,7 +1,7 @@ | |||
| --- | |||
| ################################ | |||
| # Assembly references report | |||
| # Report date/time: 2024-02-12T15:03:48.2745900Z | |||
There was a problem hiding this comment.
for some reason git created a conflict and didn't fixed itself even after merge from master. I had to manually resolve conflict and this happened.
ugras-ergun-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| MefTestHelpers.CheckTypeCanBeImported<BoundConnectionInfoProvider, IBoundConnectionInfoProvider>( | ||
| MefTestHelpers.CreateExport<ISolutionBindingRepository>()); |
There was a problem hiding this comment.
Out of curiosity: What's the line length that you try to follow?
I saw that S103 is not active on SL. Should activate it?
There was a problem hiding this comment.
We don't have a rule for that. We decide what is more readable case by case. And I don't think it should be rule-based.
There was a problem hiding this comment.
Are you fine with long lines too? For example, we have S103 configured to 200 on sonar-dotnet, to have some guidance. And we violateaccept it where it makes sense.
| var result = new List<BoundSonarQubeProject>(); | ||
|
|
||
| var bindingConfigPaths = unintrusiveBindingPathProvider.GetBindingPaths(); | ||
|
|
||
| foreach (var bindingConfigPath in bindingConfigPaths) | ||
| { | ||
| var boundSonarQubeProject = Read(bindingConfigPath); | ||
|
|
||
| if (boundSonarQubeProject == null) { continue; } | ||
|
|
||
| result.Add(boundSonarQubeProject); | ||
| } |
There was a problem hiding this comment.
IMO there are way too many empty lines in this method. It almost looks like semicolon-cr-lf-cr-lf is the command separator here.
…com/SonarSource/sonarlint-visualstudio into ue/Create-SolutionBindingRepository
| { | ||
| } | ||
|
|
||
| internal BoundConnectionInfoProvider(IUnintrusiveBindingPathProvider unintrusiveBindingPathProvider, ISolutionBindingFileLoader solutionBindingFileLoader, IThreadHandling threadHandling) | ||
| internal BoundConnectionInfoProvider(ISolutionBindingRepository solutionBindingRepository, IThreadHandling threadHandling) |
There was a problem hiding this comment.
This class could have been a method of the repo
There was a problem hiding this comment.
Repo is about read/write operations and more generic, but this class is about getting the unique connections for sloop. I would argue they are logically different things.
There was a problem hiding this comment.
Still, this could be an extension method even if you don't want to make it a part of the main interface. This piece of functionality is too small to warrant a separate class
There was a problem hiding this comment.
I don't see having a small class as a problem. This class tackles a separate problem, I might merge the functionality here with converting to SLOOP Dto's (not sure yet how it'll be)
There was a problem hiding this comment.
Merging with dtos makes more sense
| if (boundSonarQubeProject == null) { continue; } | ||
|
|
||
| result.Add(ConvertToBindingInfo(boundSonarQubeProject)); | ||
| result.Add(ConvertToBindingInfo(binding)); |
There was a problem hiding this comment.
You don't need to have the conversion if you can directly read this class from json
|
|
||
| public IEnumerable<BoundSonarQubeProject> List() | ||
| { | ||
| var result = new List<BoundSonarQubeProject>(); |
There was a problem hiding this comment.
there's no need for list if you return IEnumerable. Also, for now we don't need to list the full list of bindings with QPs, we only need the connections. So, to simplify things, I would directly parse the connection info and return just that, unless you see a future need to return the full info.
There was a problem hiding this comment.
I don't see a future use but to keep things consistent using the same object with the rest of the methods makes more sense for me.
There was a problem hiding this comment.
I don't think it's worth it to parse unnecessary data just to keep the interface slightly more clean. There are ways to minimize the difference between models and the methods used to parse them (generics, inheritance hierarchy of classes, etc.)
There was a problem hiding this comment.
Using something like Generics... etc, would be an overkill. 99% of the time, this collection will have only one member and even if there are multiple connections, it won't be hundreds or even tens of connections.
There was a problem hiding this comment.
I don't see generics as something that is too complicated to implement. And anyways, I was only using it as an example.
|
|
||
| foreach (var bindingConfigPath in bindingConfigPaths) | ||
| { | ||
| var boundSonarQubeProject = Read(bindingConfigPath); |
There was a problem hiding this comment.
Reusing the read method is good, but we might not need to load the credentials for each connection. You could create a private read method with a flag to load credentials
|
ugras-ergun-sonarsource
merged commit f70c9cc
into
feature/sloop-rule-meta-data
ugras-ergun-sonarsource
deleted the
ue/Create-SolutionBindingRepository
branch
Fixes #5210