SLVS-1433 Add credentials uri to the ServerConnection model #5660
Conversation
…aps the model from the logic model to the json model and vice-versa
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
src/Core/Binding/ServerConnection.cs
Outdated
| { | ||
| public string OrganizationKey { get; } = organizationKey; | ||
| public const string Organizations = "organizations"; |
There was a problem hiding this comment.
not sure if it was necessary to create a constant just for one usage
There was a problem hiding this comment.
I am also referencing it in the tests. I think it is better to be a constant. If we ever update this, than it will also be updated in the tests
There was a problem hiding this comment.
Should it be automatically updated in the tests? I get the argument for things like UI strings, but this feels like a value that should only change intentionally and we should strictly prohibit any unintentional changes to URIs.
There was a problem hiding this comment.
I don't think anybody is changing "unintentionally" constants in code. But sure, I can make it hard-coded instead (although hard-coded strings are usually against clean code principles)
| sonarCloud.ServerUri.Should().Be(new Uri("https://sonarcloud.io")); | ||
| sonarCloud.Settings.Should().BeSameAs(serverConnectionSettings); | ||
| sonarCloud.Credentials.Should().BeSameAs(credentials); | ||
| sonarCloud.CredentialsUri.Should().Be(new Uri(expectedServerUri, $"{SonarCloud.Organizations}/{sonarCloud.OrganizationKey}")); |
There was a problem hiding this comment.
this feels like a copy paste of the implementation. using a hardcoded string here could be safer
|
gabriela-trutan-sonarsource
merged commit b546045
into
feature/new-connected-mode
SLVS-1433
This PR targets this PR, because both of them are preparation for the server connection repository