Extending visit function's argument list from Rule.kt with parameters

[orchestr7]

What's done:

1. Extended visit function with new arguments. It becomes very useful in case you will need to match or check any parameter in any rule.
   For example - in case you will need to check a filename in a rule - you don't need to pass it through your code and change every visitor. Everything will be already passed through and you will need simply to add needed parameter to KtLint.Params data class.
2. Changed all rules and tests where visit is overridden

[orchestr7]

This is very useful for cases when you need to check file naming or any other stuff that can be added to Ktlint params.
We use this thing for our rule sets.

[orchestr7]

This will not break anything in ktlint, but will give a chance for developers of rules to use your code instead of customized forks.
Simply passing arguments to visitors and open 1 class for inheritance.

[romtsn]

This will break downstream dependencies though (like detekt) in that the public API has changed. But I guess it's fine. cc @arturbosch

[orchestr7]

@romtsn @arturbosch yeah, I understand this, but the main problem is now for usage of the framework is that we are not able to extend your library to write some rules. For example - to check everything related to file naming. Or do other checks where parameters being passed to a visitor are needed.

I am pretty sure that one day some one will need parameters being passed inside the visitors and at that time everything will be so mature that you won't be able to change it, unfortunately. So better to do that now or never :)

[orchestr7]

@romtsn @arturbosch so what do you think about breaking your API a little bit? :)

[Tapchicoma]

Your idea is interesting. While ktlint is not yet reached 1.x version, we try to introduce api changes with some deprecation strategy and keep it for few versions.

If you really want this, please consider forking and building your version of ktlint until this will be merged or introduced in some similar way.

[orchestr7]

@Tapchicoma - we have already done it and are using fork inside of our ruleset: https://github.com/cqfn/diKTat

[Tapchicoma]

@akuleshov7 what info from Ktlint.Params beside file name are you missing in the rule implementation? I would assume debug and maybe script flags?

BTW file name is already available to the rules:

ktlint/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt

Line 203 in 427e2ae

node.putUserData(FILE_PATH_USER_DATA_KEY, userData["file_path"])

[orchestr7]

@Tapchicoma I am pretty sure that you will agree with me that passing normal object of a normal class (or data class) with strong typing is much better than:

1. to use a custom map with custom string values (as .putUserData works)
2. to store parameters from argument list in each and every node (also looks to be not a good idea)

Creating objects with strong typing is much better than mapping values and keys on some magic constants.
It is much more maintainable.

Also - if you will need to pass, for example, debug argument - you would also link it to each and every node?

Today we need filePath or debug from Params, tomorrow we will need other fields from Params.
We will need to create a magic constant key each time (NEW_MAGIC_STRING_CONSTANT_KEY) and pass it to to magic map instead of passing whole object once and for all?

I guess it will be a good approach to pass Params to all rules (visitors) through the code.

[orchestr7]

@Tapchicoma @romtsn guys. if we are not able to merge this one PR - can we at least open API for RuleSet class?
#894

[romtsn]

tbh I'm fine with both of these changes, so I wouldn't mind merging this in, but I'd like @Tapchicoma and @shashachu opinions

[Tapchicoma]

Personally I don't want to break Rule class api right now. As we already discussed some parameters you want to have are already passed via user data. If you need some additional parameters, feel free to add them.

Generally I would agree that user data usage is not the best approach and we should drop it when we will introduce next iteration of Rule class with some migration strategy.

[orchestr7]

Personally I don't want to break Rule class api right now. As we already discussed some parameters you want to have are already passed via user data. If you need some additional parameters, feel free to add them.

Generally I would agree that user data usage is not the best approach and we should drop it when we will introduce next iteration of Rule class with some migration strategy.

Yes, that is fine. We are absolutely fine with merging at least #894