Misc refactors

[Sija]

No description provided.

[Sija]

@veelenga Could you please review it?

[straight-shoota]

That's a lot of changes, and they have varying decrees of impact.
I'm wondering if it might be a good idea to extract some of the non-trivial changes into separate PRs to have them stand out more?

[Sija]

@straight-shoota This might seem like a lot but there's not that much changes, actually. If you think that some particular ones deserves their own PRs, please just say so.

[veelenga]

Reviewed partially.

[Sija]

CI nightly jobs started failing with error:

Error: expected argument #2 to 'Ameba::Source#add_issue' to be Crystal::Token or Tuple(Int32, Int32), not (Crystal::ASTNode | Nil)

As far as I can see it's related to:

ameba/src/ameba/ast/variabling/assignment.cr

Line 68 in 26db705

assign.targets.find(node) do |target|

Fixed in crystal-lang/crystal#15589

[veelenga]

I would appreciate if we could extract changes in VerboseBlock into separate PR. That one looks like a tough one.

[Sija]

I would appreciate if we could extract changes in VerboseBlock into separate PR. That one looks like a tough one.

There's no point to do so, I've just shuffled the order of method definitions (so the private methods will come after the public ones), nothing really changed beside that.

[Sija]

@veelenga I'd advise you to look at the individual commits, as opposed to trying to grok the differences from the overall view - this might answer some of your questions.

[veelenga]

@veelenga I'd advise you to look at the individual commits, as opposed to trying to grok the differences from the overall view - this might answer some of your questions.

Thanks for the heads up, but there are 37 commits, I don't think it is easier to review the individual commits one by one.

[veelenga]

I would appreciate if we could extract changes in VerboseBlock into separate PR. That one looks like a tough one.

There's no point to do so, I've just shuffled the order of method definitions (so the private methods will come after the public ones), nothing really changed beside that.

Could you please clarify the reason for this change? It seems subjective and isn't enforced by any established rules, meaning it can't be guaranteed in the future. Additionally, someone else could join the project with a different perspective and revert the changes back or enforce other rules they think look better. Do you agree?

[Sija]

Could you please clarify the reason for this change? It seems subjective and isn't enforced by any established rules, meaning it can't be guaranteed in the future. Additionally, someone else could join the project with a different perspective and revert the changes back or enforce other rules they think look better. Do you agree?

Sure. That's the rule I wrote long time ago and now revisiting it after time, I'm finding it's odd having to dig through all of the private methods in order to get to the public ones. Most of other rules are written in such fashion, and I believe it's for a reason (i.e. readability). I'd happily accept a rule requiring an ordering of public/private/protected methods, yet ATM there's none. Regarding your last question: no, I don't agree since, as I've already mentioned - most of the rules are written in that fashion, and this single one was kinda an outlier.

[veelenga]

Could you please clarify the reason for this change? It seems subjective and isn't enforced by any established rules, meaning it can't be guaranteed in the future. Additionally, someone else could join the project with a different perspective and revert the changes back or enforce other rules they think look better. Do you agree?

Sure. That's the rule I wrote long time ago and now revisiting it after time, I'm finding it's odd having to dig through all of the private methods in order to get to the public ones. Most of other rules are written in such fashion, and I believe it's for a reason (i.e. readability). I'd happily accept a rule requiring an ordering of public/private/protected methods, yet ATM there's none. Regarding your last question: no, I don't agree since, as I've already mentioned - most of the rules are written in that fashion, and this single one was kinda an outlier.

Got it, thanks for explaining. However, I personally don't find this version more readable than the previous one. The change doesn't seem to offer any significant improvement from my perspective.

Also, I want to re-call what I mentioned few times during previous Misc refactoring:

1. No major benefit
2. Hard to review, takes a lot of time
3. Easy to introduce a regression

This PR contains some valuable changes and improvements but mixing it with code style suggestions, while these styles are not enforced on the project, is counterproductive IMO. Code style changes should be discussed separately, maybe written somewhere, and ideally automated with linting rules before being implemented. Otherwise, we risk spending valuable review time on subjective matters rather than focusing on functional improvements.

[veelenga]

@Sija don't want to block this one. Please feel free to finish / merge it if you think it is necessary.

[Sija]

I do, thanks for the understanding. I'm doing my best to ensure the code quality and keep the bugs and regressions to a minimum - at times by revisiting old parts of the codebase, and while I agree that some of the changes are subjective I strongly believe that such efforts are worthwhile in the long run as they're based on the (subjective, yes) care re: the readability and clarity of the code.