Configure "yes to all" per linter

[lexidor]

fixes #86
In order to make this change backwards compatible, allowYesToAll() defaults to true.
Unsafe linters may override this to false.
I also considered a default of false,
but that would entail altering all safe linters (including third-party). Unsafe linters may need to suppress must use override linter if they wish to remain compatible with previous versions of hhast.

[Atry]

Trigger CI

[Atry]

Thank you for the fix. I just wonder why do you define allowYesToAll in Linter instead of AutoFixingASTLinter, given that allowYesToAll seems meaningless for linters other than AutoFixingASTLinters.

Did you consider creating two traits LinterAllowingYesToAllTrait and LinterDisallowingYesToAllTrait and let specific linters use one of them?

[lexidor]

Thank you for the fix. I just wonder why do you define allowYesToAll in Linter instead of AutoFixingASTLinter, given that allowYesToAll seems meaningless for linters other than AutoFixingASTLinters.

Good catch! I could move this method to AutoFixingLinter (this isn't only applicable to ast ones).

Did you consider creating two traits LinterAllowingYesToAllTrait and LinterDisallowingYesToAllTrait and let specific linters use one of them?

If I did that, every autofixing linter written before the commit would need to be changed to use one of these traits. There must be a default for backwards compatibility. I chose true for the default to keep the current behavior.

I could add a LinterDisallowYesToAllTrait which contains

<<__Override>>
public function allowYesToAll(): bool { return false; }

This saves two lines of code in each unsafe linter.

[Atry]

The idea of LinterDisallowYesToAllTrait looks good to me. Do you think it would be a good idea if extracting the CLI interaction code to the AutoFixingASTLinter and LinterDisallowYesToAllTrait, instead of a function returns a boolean?

[lexidor]

AutoFixingLinter takes a generic, which means UnsafeBulkAutoFixes
can't require implements AutoFixingLinter<Terror> without being generic itself.
It doesn't use the generic either, so this small marker interface is used instead.

[Atry]

By the way, technically we could avoid the <<__Override>> and still keep backward compatibility by introducing a new base abstract class of AutoFixingASTLinter like:

abstract class AutoFixingASTLinter extends BaseAutoFixingASTLinter {
  use LinterAllowingYesToAllTrait;
}

[lexidor]

Do you think it would be a good idea if extracting the CLI interaction code to the AutoFixingASTLinter and LinterDisallowYesToAllTrait, instead of a function returns a boolean?

💭 I am not so sure. Autofixing linters don't know about the CLI at all right now. They do know about LSP though... 🤔 Maybe adding CLI knowledge isn't that bad. Feels like the responsibility of something else though.

[lexidor]

By the way, technically we could avoid the <<__Override>> and still keep backward compatibility by introducing a new base abstract class of AutoFixingASTLinter like:

abstract class AutoFixingASTLinter extends BaseAutoFixingASTLinter {
  use LinterAllowingYesToAllTrait;
}

I prefer adding a virtual method over adding a new base class. The gain of not having to use <<__Override>> is outweighed by the cost of needing to know more types.

AutoFixingASTLinter is not the only autofix linter around. AutoFixingLinter has three children: AutoFixingASTLinter, AutoFixingLineLinter, (final class) NewlineAtEndOfFileLinter.

So we would need to add BaseAutoFixingASTLinter and BaseAutoFixingLineLinter.

[Atry]

If I understand correctly, c63e6d5 creates two traits and breaks backward compatibility. Did you change your mind?

[lexidor]

If I understand correctly, c63e6d5 creates two traits and breaks backward compatibility. Did you change your mind?

The creation of the second trait doesn't break bc. Linters that are unchanged still inherit the default areBulkAutoFixesSafe() from AutoFixingLinter.

Did you consider creating two traits LinterAllowingYesToAllTrait and LinterDisallowingYesToAllTrait and let specific linters use one of them?

If I did that, every autofixing linter written before the commit would need to be changed to use one of these traits. There must be a default for backwards compatibility. I chose true for the default to keep the current behavior.

I could have been more clear with my response here. I (mis)read your question as, "Why not create two traits and all autofixing linters use either one or the other?". We can create two traits just fine (hhast owns the namespace). We just can't require all linters to use one of these traits.

I am sorry for any confusion I may have caused. I was answering a different question than the one you had asked me (probably).

[Atry]

Looks good to me! Thank you!