Skip to content

SLVS-1481 Show message when code does not match#5727

Merged
gabriela-trutan-sonarsource merged 21 commits intofeature/fix-suggestionfrom
gt/not-match-fix-suggestion
Oct 7, 2024
Merged

SLVS-1481 Show message when code does not match#5727
gabriela-trutan-sonarsource merged 21 commits intofeature/fix-suggestionfrom
gt/not-match-fix-suggestion

Conversation

@gabriela-trutan-sonarsource
Copy link
Copy Markdown
Contributor

@gabriela-trutan-sonarsource gabriela-trutan-sonarsource commented Oct 4, 2024

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod bot changed the title SLVS-1481 Show message when code does not match SLVS-1481 Show message when code does not match Oct 4, 2024
…log.

This has multiple advantages including better testability of code and the fact that the callers don't have to add references to all the services required to show the ManageBinding dialog,
Base automatically changed from gt/implement-fix-suggestion to feature/fix-suggestion October 7, 2024 11:21
Comment on lines +116 to +117
var textView = GetFileContent(parameters, absoluteFilePath);
if (!ValidateFileExists(textView, absoluteFilePath))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor:
The GetFileContent and the ValidateFileExists should most of the times be called together. They could be combined in a single method, as we also do for the ValidateConfiguration.
Something like: ValidateFileExists(absoluteFilePath, out var textView)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. Getting the file content and validating if it exists should be separate, otherwise it violates the SRP (single responsibility principle).

The same with ValidateFileExists(absoluteFilePath, out var textView). The out parameter is already an indicator that the method does more than it should: validating and returning something.

I will leave them separate for now.

@gabriela-trutan-sonarsource gabriela-trutan-sonarsource marked this pull request as ready for review October 7, 2024 14:55
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Oct 7, 2024

@gabriela-trutan-sonarsource gabriela-trutan-sonarsource merged commit cae02cb into feature/fix-suggestion Oct 7, 2024
@gabriela-trutan-sonarsource gabriela-trutan-sonarsource deleted the gt/not-match-fix-suggestion branch October 7, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants