bscan / PerlNavigator

Perl Language Server that includes syntax checking, perl critic, and code navigation
MIT License
198 stars 39 forks source link

tidyWrapper.pl: use Perl::Tidy::Sweetened if available #91

Closed davenonymous closed 6 months ago

davenonymous commented 1 year ago

Perl::Tidy::Sweetened adds support for some syntactic sugar provided by some modules. The module uses Perl::Tidy's prefilter and postfilter hooks to support method and func keywords (which are also supported on more recent versions of Perl::Tidy) and other stuff.

tidyWrapper.pl is now additionally checking for the availability of the addon module and switches out the perltidy method that's being used for tidying.

We are using the builtin universal can method to get references to the given sub so we can call them easily later.

More details: https://metacpan.org/pod/Perl::Tidy::Sweet https://perldoc.perl.org/UNIVERSAL#CLASS-%3Ecan(-METHOD-)

oalders commented 1 year ago

I wonder if this could be enabled with a switch? It might be surprising if this gets used just because it's available.

davenonymous commented 1 year ago

I wonder if this could be enabled with a switch?

I initially wanted to add a VSC setting that allows choosing which tidy script to use, so that users can utilize any formatter. But that would have implied a lot of changes in the code, and I wanted to keep it simple for now.

bscan commented 1 year ago

Thanks for the pull request! I like the idea of having better support for keywords and newer syntax. The Navigator does something similar for Perl::Critic to allow usage of methods, classes, async subs, etc. Otherwise, PPI doesn't correctly parse the document. Similarly, the Navigator has extra syntax highlighting for classes and methods, and explicitly supports them in outlines and go-to definition.

However, Perltidy always seemed far more robust to pluggable keywords and new syntax, and Perltidy development is more active than PPI and the TextMate grammars. Do you have some examples where the behavior is different when using Perl::Tidy::Sweetened?

If we want to add a parameter to control this behavior, I'm happy to help out adding the extra parameters. The critic wrapper takes a few different parameters currently (e.g. severity), and this could work in a similar fashion.

bscan commented 1 year ago

I was curious so I did some tests. The docs mention methods and subs signatures, so I tried it on 5.38 classes, but I did not see any differences.

However, I see a curious behaviour with Moops. I used this example with everything on a single line. use Moops;role NamedThing {has name => (is => "ro", isa => Str);}class Person with NamedThing;class Company with NamedThing;class Employee extends Person {has job_title => (is => "rwp", isa => Str);has employer=> (is => "rwp", isa => InstanceOf [ "Company" ]);method change_job (Object $employer, Str $title ) { $self->_set_job_title($title); $self->_set_employer($employer); }method promote (Str $title ) {$self->_set_job_title($title);}}

Tidying with or without Perl::Tidy::Sweetened (and no profile) gives the exact same result:

use Moops;
role NamedThing { has name => (is => "ro", isa => Str); } class Person with NamedThing;
class Company with NamedThing;
class Employee extends Person {
    has job_title => (is => "rwp", isa => Str);
    has employer  => (is => "rwp", isa => InstanceOf [ "Company" ]);
    method change_job (Object $employer, Str $title ) { $self->_set_job_title($title); $self->_set_employer($employer); }
    method promote    (Str $title )                   { $self->_set_job_title($title); }
}

However, if you Tidy again, the results change with Sweetened while regular Perl::Tidy will not. I know the perltidy flag --iterations=n exists to handle these cases, but it's still odd to see. After 3 iterations it converges to the following result, which is a slight improvement over the base:

use Moops;
role NamedThing { has name => (is => "ro", isa => Str); }

class Person with NamedThing;

class Company with NamedThing;

class Employee extends Person {
    has job_title => (is => "rwp", isa => Str);
    has employer  => (is => "rwp", isa => InstanceOf [ "Company" ]);
    method change_job (Object $employer, Str $title ) { $self->_set_job_title($title); $self->_set_employer($employer); }
    method promote (Str $title ) { $self->_set_job_title($title); }
}

Perhaps Sweetened needs to be run with multiple iterations, either directly in Perltidy via the iterations param, or from the tidyWrapper? In most of the cases I tried, classes, methods, and signatures seem to be handled fine by standard perltidy, or left alone. Perhaps I need more a compelling example? There aren't any examples on the Perl::Tidy::Sweetened Github page.

davenonymous commented 1 year ago

Hmm, you are right. My original problems went away with the more recent Perl::Tidy versions. Maybe Sweetened isn't really necessary after all anymore. I've opened an issue in the Sweetened repository asking about this.

That it needs to run multiple times is weird.

mvgrimes commented 1 year ago

Perl::Tidy added support for the new (experimental) class, method, field and ADJUST keywords in version 20230309 and support for signatures in an earlier version. This is great and means we can delete code from ::Sweetened.

If you are looking to support only the new builtin syntax, then you might not need Sweetened.

On the other hand, Sweetened might be helpful if you want to support some of the alternate class implementations that have emerged over the years (Method::Signatures::Simple, MooseX::Method::Signatures, MooseX::Declare, Moops, Kavorka, Future::Async, Object::Pad, etc). I'll likely do some work in the coming weeks to explore what is covered by new versions of Tidy and where Sweetened is still needed.

bscan commented 6 months ago

I'll close this for now, but definitely feel free to re-open in the future. We'll just also need an option to allow us to turn this on and off.