crystal-ameba / ameba

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

Make it easier to add issues for nodes with name location preference #422

Closed Sija closed 11 months ago

Sija commented 11 months ago

@veelenga This PR fixes CI, btw

straight-shoota commented 11 months ago

Have you considered a different method name (such as add_name_issue)? I would argue that's a more explicit separation of behaviour and better than a parameter.

Sija commented 11 months ago

@straight-shoota I did considered some, yet it was the best I could come up with atm. I prefer named parameter, since it's a preference only, I see no point of having a dedicated method for it.

straight-shoota commented 11 months ago

I would say the callsite is more succinct and easier to understand what it means. prefer_name_location is named after an internal detail, but naming_issue_for describes the purpose.

naming_issue_for node, MSG
#vs
issue_for node, MSG, prefer_name_location: true
veelenga commented 11 months ago

I agree with @straight-shoota that naming_issue_for looks more understandable. Also, that amount of named params in the issue_for method makes it much more complicated.

Sija commented 11 months ago

Yeah, I agree, although in this case the parameter indicates a preference, not an unconditional behavior, like I've understood you've suggested. Also, naming is a pretty generic term, and doesn't reference _namelocation, which this feature is all about.

Sija commented 11 months ago

In this particular context all proposed names: i.e. add_name_issue and naming_issue_for are IMO too vague and unclear. name_location is a term already used, therefore familiar. Also, as I mentioned before, this is a setting which modifies the original behaviour, and so in my opinion is not suited for extraction to a different method.