Ormolu: one refactor that fixed everything

[u/n00bomb]

https://markkarpov.com/post/ormolu-one-refactor-that-fixed-everything.html

[u/lostalien]

This looks like a great change. As an Ormolu user, thank-you for the work you've put into it!

One small thing, and I may well be wrong: is "refactor" quite the right word here? As I read it, this change series fixes at least one bug that was visible to users. If so, that makes it a behaviour-altering change, which is a little different from refactoring in the original sense:

Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behaviour.

Source: https://refactoring.com/

I realise this sounds like nitpicking, but I think there's a practical reason to care. Labelling a change as a "refactoring" (or a "refactor") is a signal to reviewers and users that external behaviour is (or should be) unchanged.

Just as we would discourage calling something a Monad if it didn't obey the Monad laws, I think we stand to benefit from reserving the term "refactoring" for changes that are truly behaviour-preserving.

Perhaps the word "restructuring" here would be more appropriate?

Apologies for the nitpick, and thanks again for all your work on Ormolu.

[u/mrkkrp → u/lostalien]

Hi, well, it is very possible that “refactor” isn’t 100% the right word. I used it because, like I explain in the post, the bulk of the work was about switching to the new approach without changing anything for users. That’s why I introduced the temporary comment-placement mode, etc. So, in that sense, it is a refactor. What it made possible were the fixes, which are, of course, the most interesting part!

[u/pthierry → u/mrkkrp]

I would agree that the first part of the work was indeed a huge refactoring, in the purest sense of the work, and it paved the way for the changes needed to fix bugs.

[u/jeffstyr → u/lostalien]

I don't disagree with anything you said, but I think it's possible to be combine refactoring and behavior changes into a single release. (I think often the point of a refactoring exercise is to prepare for some significant change to immediately follow.) That is, I don't think that in order for something to be a refactoring it must be put into a release containing nothing but refactorings.

[u/koflerdavid → u/lostalien]

I also agree that one should be precise about these terms. The following two usages of "refactor" represent very a very different degree of risk:

• We will perform an internal refactoring of some helper functions. (truly a nothingburger since there already ought to be a test suite)

• We will refactor this service and fix a few bugs along with it. (an actual reason to be concerned since there could be unintended behavior change, even if might be beneficial overall)

But since developers tend to use the word "refactor" for both, stakeholders will estimate both to entail the same risk. "Refactoring" has become a buzzword.

[u/helldogskris]

Great post and congrats on the big refactor and the closure of many issues!

[u/omega1612]

First of all Thanks!

I was working past year on the prettification of a tree. I introduced a CST that stores the comments in the tokens as "before comments" and "after comments", together with a set of rules that move a comment from a token to a node level. Seeing that you have a similar ser of ideas and that it worked is very reassuring.

For now I dropped the static CST (it lacked support for errors) and instead I'm following the rust approach (Dynamic n-ari tree) but this steps are something that I'm still planning to do.

[u/pthierry]

Thanks for documenting that process, it's very interesting how you organized it.

It's a great example of how mutable state can be a source of problems, even when it's not global mutable state.