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

Format (-F) command is not honouring the baseline file #1072

Closed
girish3 opened this issue Jan 29, 2021 · 8 comments
Closed

Format (-F) command is not honouring the baseline file #1072

girish3 opened this issue Jan 29, 2021 · 8 comments
Milestone

Comments

@girish3
Copy link

girish3 commented Jan 29, 2021

I created a baseline file from the below command,
ktlint --baseline=ktlint-baseline.xml

and I could see no errors are reported if I run it again since there are no changes against the baseline file. But when I add the format flag, the baseline file is getting ignored and the terminal spit out the fixed errors,

ktlint --baseline=ktlint-baseline.xml -F does not work as expected!

Can anyone help?

@romtsn romtsn added the bug label Mar 8, 2021
@Tapchicoma
Copy link
Collaborator

Current baseline implementation does not prevent linting, but rather it just suppresses found errors.

So in case of -F flag - format is still happening and, if non-formattable error present in the baseline still occurs, it will be printed. Another issue with format and baseline - it relies on error position in text file, so when format changes Kotlin file, errors positions may be updated.

@androiddevcoding
Copy link

I also ran into this inconvenience. I am looking for a workaround.

@Kardelio
Copy link

same! ^^^ would love if we could filter formatting with the baseline file

@paul-dingemans
Copy link
Collaborator

same! ^^^ would love if we could filter formatting with the baseline file

Please feel free to contribute a PR ;-)

@Goooler
Copy link
Contributor

Goooler commented Mar 27, 2023

@paul-dingemans Could you validate this issue? I believe it should be fixed in 0.49.0-SNAPSHOT.

@paul-dingemans
Copy link
Collaborator

@paul-dingemans Could you validate this issue? I believe it should be fixed in 0.49.0-SNAPSHOT.

Unfortunately it is not solved. Also, I have some major doubts whether we will be able to solve it in a good way.

The baseline functionality is functionality that lives only in Ktlint CLI. So the KtlintRuleEngine and the Rules don't know anything about the baseline.

An interesting idea would be to introduce a new kind of callback which "asks" permission to autocorrect a LintError before actually applying a fix. API Consumers could use this functionality for a kind of step-through linting/formatting where the user can determine per case whether the fix should be applied, or the API consumer could try to automatically add @Suppres("ktlint:rule-id") annotations. This would be a breaking change and all rules need to be accomodated for that.

But this will not solve the issue with the Ktlint CLI Baseline. The baseline contains information about a line number, an offset on the line and a rule-id. This location does not necessarily matches with the current location of the node that caused the issue when the baseline was created. Reasons for this:

  1. The code above the error location was altered by adding or removing lines. Upon invoking Ktlint again, the error location already does not match anymore. When using the baseline functionality with pure linting, this also results in the behavior that the base does not seem to be respected.
  2. Some node in the AST which was processed before the error node altered the code by adding or removing lines. Upon start of Ktlint the error location migth have been pointing at the expected node but due to formatting prior nodes, it is no longer found at the expected position.

@paul-dingemans
Copy link
Collaborator

This issue should not be marked as bug. It can be seen as an enhancement. A possbile way to solve would be as follows:

  • pass baseline to KtlintRuleEngine

  • parse code to AST as usual

  • Dynamically add a suppress annotation of a new type (for example @BaselineSuppress("ktlint:rule-id")) to the AST. Add this new type of annotation to the SuppressionLocatorBuilder.

  • lint/format the code as usual.

    • Errors for code suppressed in the baseline will no longer be reported due to the dynamically added suppression.
    • In case some other line of code in the files is autocorrected, the error which was suppressed in the baseline will not be affected anymore because of the dynamically added suppression.
  • In format function:

    • Update baseline. Due to format of lines of code which were not suppressed, the line/column of baseline errors might have changed.
    • when writing the output in the format function, filter out the dynamically added suppression (e.g. remove all @BaselineSuppress annotations.

Introduction of the annotation above also have other implecations that need to be mitigated:

  • Dynamically adding the annotation affects the line numbers of errors on lines below that annotation
  • Special care has to be given to the AnnotationRule. Dynamically inserting an annotation with an argument @BaselineSuppress("ktlint:rule-id") on top of a construct that was already annotated might cause all annotations to be wrapped to separate lines. This is unwanted because the annotation will be not be persisted.

This change will require a considerable refactoring. I will not implement it myself as I am not convinced that it is worth to maintain the baseline functionality in the long term. I am a strong believer that a project that implements ktlint also should take the effort to resolve existing problems.

@paul-dingemans paul-dingemans added Parked Issue is not likely to be solved or has no clear resolution yet and removed bug labels Jul 23, 2023
@paul-dingemans
Copy link
Collaborator

Fixed by #2671 for rules that implement the new RuleAutocorrectApprovalHandler interface which lets the API Consumer (in this case Ktlint CLI) decide which LintError is to be autocorrected, or not. When using in combination with external rulesets, that have not yet implemented the new RuleAutocorrectApprovalHandler interface, the behavior remains unchanged.

@paul-dingemans paul-dingemans added this to the 1.3.0 milestone May 28, 2024
@paul-dingemans paul-dingemans removed the Parked Issue is not likely to be solved or has no clear resolution yet label May 28, 2024
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

No branches or pull requests

7 participants