Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS-363 - Support environment variable for setting skipNodeProvisioning #4912

Merged
merged 5 commits into from
Nov 20, 2024

Conversation

ericmorand-sonarsource
Copy link
Contributor

@ericmorand-sonarsource ericmorand-sonarsource commented Nov 19, 2024

@ericmorand-sonarsource
Copy link
Contributor Author

ericmorand-sonarsource commented Nov 19, 2024

This is how it looks like in SQ:

image

@ericmorand-sonarsource ericmorand-sonarsource force-pushed the JS-363 branch 3 times, most recently from b18b79f to 589ca3a Compare November 20, 2024 08:33
assertThat(propertyDefinition.category()).isEqualTo("JavaScript / TypeScript");
assertThat(propertyDefinition.subCategory()).isEqualTo("General");
}

private List<PropertyDefinition> properties() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the opportunity to rewrite this utility method using a more declarative and concise syntax.

*
* Note that this method should probably not be hosted here: either it should be part of a dedicated helper class, or it should be provided by a Markdown-to-HTML library. Since it is only used in this specific class, it is acceptable for now to have it hosted here.
*/
protected String getHTMLMarkup(String markdownMarkup) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not declare this as private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind, actually, Do you think I should declare the property as private?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say that if you're not using it outside of the class, it might as well be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the visibility to private, but SQ requests me to set is as static. Will do.

import org.sonar.api.SonarEdition;
import org.sonar.api.SonarQubeSide;
import org.sonar.api.SonarRuntime;
import org.sonar.api.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we had a rule that forbade to use star imports in java. I don't mind this, just raising this for information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my IDE did that for me. Let me fix.

EDIT: Actually, it does not seem to trigger an issue, from the quality gate result. Maybe the rule is not in SonarWay anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about not using star imports as seems to be the convention? We should somehow enforce it, either with a rule or an IDE config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind. I have no idea what problem using start import could bring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

sonarqube-next bot commented Nov 20, 2024

@ericmorand-sonarsource ericmorand-sonarsource merged commit f085d38 into master Nov 20, 2024
16 of 17 checks passed
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