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

move sensors to own library #1099

Merged

Conversation

jmecosta
Copy link
Member

separate all sensors into own library, connect the cxx plugin to it.

@jmecosta jmecosta requested review from guwirth and Bertk March 27, 2017 09:40
@jmecosta
Copy link
Member Author

@guwirth this is a initial implementation. for now only preparatory work to take the sensors out into a reusable library. once this is ready i will create a dedicated pr for c plugin

@jmecosta jmecosta force-pushed the feature/split-c-into-own-plugin branch from ec3434f to 083deac Compare March 28, 2017 10:50
move files

move project builder
@jmecosta jmecosta force-pushed the feature/split-c-into-own-plugin branch from 5d9c6b2 to e11c793 Compare March 28, 2017 11:18
@jmecosta
Copy link
Member Author

@guwirth this is ready, i think

@jmecosta
Copy link
Member Author

massive refactoring, without touching integration tests and everything works :)

@guwirth guwirth added this to the 0.9.8 milestone Apr 1, 2017
@guwirth
Copy link
Collaborator

guwirth commented Apr 1, 2017

@jmecosta Sounds good will have a look...

@guwirth
Copy link
Collaborator

guwirth commented Apr 1, 2017

@jmecosta looking over the code without testing it it looks good.

  • Question: Is this backward compatible? Using the new version will users loose their history?
  • How will this look like in UI? Two separate sensors C++ plugin and C plugin?

@@ -293,4 +334,11 @@ private int saveViolations(InputFile inputFile, SourceFile squidFile, SensorCont
public String toString() {
return getClass().getSimpleName();
}

//@Override
//public List<Metric> getMetrics() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


@Override
public String[] getFileSuffixes() {
return new String [] {"cpp", "hpp", "h", "hxx", "cxx"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

is getSourceFileSuffixes + getHeaderFileSuffixes?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, but it takes less code to do this. we on the next pr we should clarify a bit better this behaviour

@@ -48,7 +50,7 @@ public void test() {
RulesDefinition.Repository repository = context.repository(REPOSITORY_KEY);

assertThat(repository.name()).isEqualTo(REPOSITORY_NAME);
assertThat(repository.language()).isEqualTo(CxxLanguage.KEY);
assertThat(repository.language()).isEqualTo("c++");
Copy link
Collaborator

Choose a reason for hiding this comment

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

'c++' hard coded. Is there no function?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -167,7 +177,7 @@
private static List<PropertyDefinition> codeAnalysisProperties() {
String subcateg = "(2) Code analysis";
return new ArrayList<>(Arrays.asList(
PropertyDefinition.builder(CxxCppCheckSensor.REPORT_PATH_KEY)
PropertyDefinition.builder("sonar.cxx." + CxxCppCheckSensor.REPORT_PATH_KEY)
Copy link
Collaborator

Choose a reason for hiding this comment

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

hard coded string 'sonar.cxx': better use a constant

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@jmecosta
Copy link
Member Author

jmecosta commented Apr 1, 2017

@guwirth this pr should not introduce any regression. this is meant only as a preparatory work to introduce the c plugin. That will go in once this is merged.

when we have the c plugin, the only regression ive identified was related with

"sonar.cxx.suffixes.sources" and headers, basically if we dont change the prop name then all users that use both plugin will need to manually modify their values to remove the c extensions.

if changing the name of the prop and include only the c++ prefix then you are forcing people to use the 2 plugins.

For me to reduce regression, i would not change the prop nor the prefix. And mention in documentation that if a c plugin is to be used then you must update the default extensions in the c++ plugin only to include c++ extensions.

@jmecosta
Copy link
Member Author

jmecosta commented Apr 2, 2017

@guwirth should i merge then?

@guwirth
Copy link
Collaborator

guwirth commented Apr 2, 2017 via email

@jmecosta jmecosta merged commit 3669b01 into SonarOpenCommunity:master Apr 2, 2017
@guwirth guwirth mentioned this pull request Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants