Explore using CheckstyleAutoFix parent recipe instead of individual recipes

[Anmol202005]

Current Implementation

Tests currently use individual recipes, which creates complex testing logic.

Investigation Needed

Explore how can we simplify by using the existing CheckstyleAutoFix recipe.

Current Issue:

CheckstyleAutoFix recipe accepts path of violation report and configuration.

[Anmol202005]

I have two ideas here:

• we can update the checkstyleAutoFix to take List<CheckstyleViolation> and CheckConfig
  and make a new main recipe that takes path to violation report and config path and parses them and calls the CheckstyleAutoFix recipe from there.

• we can also test recipe similar to checkstyleAutoFix where it takes List and CheckConfig as params

i prefer the 1st one.

@rdiachenko @timurt what's your insight ?

[rdiachenko]

@Anmol202005

Option 1 adds another layer, which means more complexity to deal with. I don't see clear benefits of doing this.

Could you clarify Option 2 and give more details, not sure I fully understand it?

We also have Option 3, right? This is to use CheckstyleAutoFix without modification and instantiate it with report and config path. We already have report path, as we generate it in the test class during Checkstyle execution. And we have a config that we use to run Checkstyle. This config is in xml format in Input file headers. Can we store it into temp directory and use its path during test execution?

[Anmol202005]

@rdiachenko

We also have Option 3, right? This is to use CheckstyleAutoFix without modification and instantiate it with report and config path. We already have report path, as we generate it in the test class during Checkstyle execution. And we have a config that we use to run Checkstyle. This config is in xml format in Input file headers. Can we store it into temp directory and use its path during test execution?

Not sure how we can are going to convert the config to string and use it as File. may require a lot of workaround.

Could you clarify Option 2 and give more details, not sure I fully understand it?

A test recipe similar to CheckstyleAutoFix that takes Violations and config directly rather than paths.
exactly same recipe just we remove the parsing logic and only contains getRecipe method.

[rdiachenko]

Not sure how we can are going to convert the config to string and use it as File. may require a lot of workaround.

you already have access to input files, the simplest way to read it and get its config part as a string. I'd also suggest to check related code in Checkstyle test how they retrieve it

[timurt]

we can also test recipe similar to checkstyleAutoFix where it takes List and CheckConfig as params

As far as I understood, the plan is to have another bean CheckstyleTestAutoFix that extends CheckstyleAutoFix, have custom initialziation logic that uses List<CheckstyleViolation> and CheckConfig and replace the main CheckstyleAutoFix with an alternative implementation for test purposes

Sounds reasonable to me

[Anmol202005]

we can also test recipe similar to checkstyleAutoFix where it takes List and CheckConfig as params

As far as I understood, the plan is to have another bean CheckstyleTestAutoFix that extends CheckstyleAutoFix, have custom initialziation logic that uses List<CheckstyleViolation> and CheckConfig and replace the main CheckstyleAutoFix with an alternative implementation for test purposes

Sounds reasonable to me

@rdiachenko Sounds good to me too, what's your thought.

Although it can be much simpler no need to extend CheckstyleAutoFix.
Something like this will work IMO:

 @Override
    public List<Recipe> getRecipeList() {

        return CheckstyleRecipeRegistry.getRecipes(violations);
    }

[rdiachenko]

@Anmol202005

@rdiachenko Sounds good to me too, what's your thought.

What's the result of this investigation #67 (comment) ? I don't see any details

[Anmol202005]

What's the result of this investigation #67 (comment) ? I don't see any details

@rdiachenko We can go this way, we will need to create a method to read the config as String from the input and then pass it as a File to the recipe as u said.

I don't see a predefined method for this.

[rdiachenko]

@rdiachenko We can go this way, we will need to create a method to read the config as String from the input and then pass it as a File to the recipe as u said.

this should be simple, same approach as with the report that we store as a temp file. Please, reaise a draft PR and show how this approach looks

[Anmol202005]

this should be simple, same approach as with the report that we store as a temp file. Please, reaise a draft PR and show how this approach looks

Stuck with header config, we provide it manually :(

[Anmol202005]

Stuck with header config, we provide it manually :(

@rdiachenko works good for upperEll.