search

crystal-ameba / ameba

A static code analysis tool for Crystal
https://crystal-ameba.github.io
MIT License
514 stars 35 forks source link

Make `RuleConfig#properties` accept only `Call` nodes #436

Closed Sija closed 8 months ago

Sija commented 8 months ago

This PR removes the support for other ways of defining properties than using a call syntax - i.e. assign and type declaration.

It also adds an optional named argument as, in order to specify the property type when needed (e.g. for nil-able properties).

With this change there's no need for # ameba:disable Lint/UselessAssign pragmas in #430.

straight-shoota commented 8 months ago

I find this is a bit questionable. Seems like shoehorning this change to enable another one. But looking at it in isolation, I think assigns are quote nice as an idiomatic syntax for property declaration.

And I don't think it can be a realistic solution to avoid rewriting assign syntax in macros everywhere. .

Sija commented 8 months ago

I find this is a bit questionable. Seems like shoehorning this change to enable another one.

This actually enabled more streamlined API. Previously, you'd have 3 different ways to achieve the same. Two of these were equivalent (foo = 1 and foo 1), and the third (foo : Int32? = 1) allowed to provide an explicit type - which is what this PR implements, aside of removing the other two ways. So as you can see this change was overdue since long time already, and was not done just to remove few ameba:disable pragmas as you suggested.

Having 2 or 3 different ways to achieve the same is also an anti-pattern in Crystal world, isn't it?

But looking at it in isolation, I think assigns are quote nice as an idiomatic syntax for property declaration.

Yeah, I've already voiced my concerns re: usage of type declarations in DSLs in https://github.com/crystal-ameba/ameba/pull/430#issuecomment-1854999845. And re: the syntax - 2 rules out of 77 were using type declarations anyway, so it was unused anyway 🤷‍♂️ Also, I remember you've been advocating for this change to happen, so it's funny to see you've done 180 😅

And I don't think it can be a realistic solution to avoid rewriting assign syntax in macros everywhere.

No, it's not. Also, you're commenting the changes done in #430 and not this PR really.

straight-shoota commented 8 months ago

I'm not critizing standardization on a single syntax, but the choice of that syntax. It was the most used, but IMO assign / type declaration might've been a better choice overall.

Also, I remember you've been advocating for this change to happen, so it's funny to see you've done 180 😅

Did I? 🤔 Opinions can change 🤷

Sija commented 8 months ago

Fine with me :) I prefer this one, the beauty is in the eye of the beholder as they say - what's even more true in the land of DSLs 😀