RFC: Always trying to get.. Betterer

Jon Allured, Pavlos Vinieratos and George Kartalis talk about Betterer, a tool by Craig Spence (aka phenomnomnominal), all about trying to get your codebase in a better state. We used it for a couple of week. What did we think? What did we love, and what did we had issues with? Let us guide you through our experience with betterer.

PS: Jon loves green dots.

Links:
- https://github.com/phenomnomnominal/betterer
- https://github.com/artsy/eigen/pull/8045

Transcript

Jon Allured: 0:07

Hi, everyone, welcome to another episode of Artsy Engineering Radio. I'm Jon. I'm kind of like a host today. But really, I'm joined by two people that will do most of the talking. It's Pavlos and George. Pavlos, why don't you start by introducing yourself?

Pavlos Vinieratos: 0:20

Hello, hello. Yeah, my name is Pavlos, like I said, I'm one of the engineers here at Artsy. I'm in the mobile platform team. So that means that we basically work on both apps that we have, and trying to make sure that every dev is welcome to these repos. They can change stuff easily, George.

George Kartalis: 0:42

Thanks. Yeah. So I'm George. I'm also an engineer here at Artsy. And I work on the Find and explore team where I'm focused mostly on front end mobile related stuff. And we focus as a team on search related topics, and also the create alert the alert ecosystem and stuff like that.

Jon Allured: 1:01

Yeah, how would people find artwork that they're interested in. Cool. Yeah. So this is one of those episodes where it's sort of a little bit freeform, we call them RFCs. But we did have a couple ideas about things to discuss. But let's start by maybe describing this repo Eigen. Eigen is the repo that is for our mobile applications for our Android and our iOS applications. It's a React Native project. It's been around for a long time, gone through some different cycles of, you know, like, how it was set up, how like, you know, how CCI was done, etc. And one of the things that we want to talk about is the use of like, a change that we've been making and trialing out to make things better. What's it called, again, Pavlos?

Pavlos Vinieratos: 1:50

Very well named. Called Betterer. Yeah, betterer. Exactly. So it's like prettier, but with an extra -er at the end.

George Kartalis: 2:00

Yeah. Way betterer than better, exactly. Yeah.

Jon Allured: 2:05

So this is, this is a TypeScript library. Correct?

Pavlos Vinieratos: 2:09

So this total, that's, yeah, I'm pretty sure it's written in TypeScript. And it's, it's basically trying to make sure that it's trying to make it easy for devs to track things in numbers. So it can help you track things that you want to remove, let's say some comments, or like console logs, or whatever you want to remove, it's easy to just tell that tool, like this is the string I want to remove. And then it counts how many you have. And then you know, next time, it will tell you like, oh, you removed two of them great. Or you added one, like, Are you sure? That kind of thing. And then you can also do the opposite. You know, if you want to have like 3000 new files, or whatever you can, you can tell it like, this is the thing I'm measuring, either try and go to infinity or try and go to zero, basically, and it will track this for you.

Jon Allured: 3:01

Yeah, this is on GitHub under phenomnomnominal or something. If you just search Google for betterer, I'm sure you'll find it. I happen to look at the Git history and see that you have the most latest change here Pavlos, update changelog.

Pavlos Vinieratos: 3:17

Yeah, what's if you see, I'm pretty sure it's a typo PR. I have tried to make a PR, like an actual PR, you know, more than just a typo. And the code was not complicated, but not the easiest. So I haven't finished it yet. But it's something that we we found while trying to use it here. It is why we wanted to talk about this basically. So maybe maybe I can summarize. Or I can say that before or before last week, I guess we had another tool called danger. And I think this works with JavaScript and Ruby. And it's basically a similar tool where you can give it you know, it takes a PR as a as the input. And then it can tell you or you can do you know any kind of calculations you want. So if you want again, some specific string some like imports or something to not be there, it will count and it will tell you like oh, you added one, are you sure? Or you know, is it okay? Or is it not okay, basically, but it will not tell you when you remove something, and then it's kind of silent. So the idea was betterer was, you know, first of all to have this kind of more vocal, I guess, when you know, we're doing good things by removing things that we don't want from our codebase. And then the second thing was that danger was mostly or not mostly it was only running on CI only running circle CI that we use, just in general, it's kind of easier to set this up on CI so you know oftentimes, like it has happened to me and I'm sure to both of you. You know, I added an import of like, moment that we don't want, for example, and then after I'm done and I push everything and I'm ready, I get this notification on GitHub that says, we don't use that anymore, you know, and then I have to take it out again and like remove this blah, blah, blah. So it would be nicer to get this feedback immediately while you're still working on this, which again, better help there. Does that make sense?

Jon Allured: 5:21

Yeah, no, I think that's great. So So for example, if we had a danger rule that was like, no comments that are like TODO, or something like that. You would like fail, ci would fail your pull requests, and you'd see some comments saying, Hey, you violated this rule, here's what you can do about it. But that always happened after a cycle on circle. And so the feedback loop wasn't as tight as being able to run betterer locally, and see that feedback sooner before you even maybe open the pull request. Do, I have that right?

Pavlos Vinieratos: 5:52

Yeah, exactly. George have you had any danger like, disagreements ever? While you're here?

George Kartalis: 5:59

No, it's it was the same for me mostly, like I would love to be to give me like more instant feedback before like having to do another commit to correct whatever was wrong.

Pavlos Vinieratos: 6:13

Exactly. So yeah, that was kind of the initial reason that I wanted to put that total and replace danger. Or if you know, if they could live together, that's fine. One thing that danger does that betterer doesn't is That danger has, so betterer basically, the the input of betterer is your code, like your whole repo, right? It doesn't, it can check the changes between your branch and like the main branch, but it has the whole repo as the input, whereas danger has the PR, but also the repo, but also the org. So for example, we have rules on danger that say like, if the title of the PR is not, you know, obeying some rules, then it will give you a notification, or anything like that. So it can definitely do more things. But it's specifically for the rules about, you know, imports and some code stuff. That's what I was trying to replace betterer, or bring betterer instead of that.

Jon Allured: 7:14

So it's like making a little separation between the kinds of roles that are org wide at the PR level, whatever, like require danger's kind of approach to these kinds of rule enforcements versus something that could be surely run locally, and you would know sooner. And one other thing you were talking about was the way the rules are defined, that there's like, the way that betterer, you know, like, the way you declare a rule, maybe you could talk a bit more about that.

Pavlos Vinieratos: 7:43

Yeah, so that's one of the one of the benefits of betterer, it's basically very easy to add these kind of rules, this kind of go regex with a string, and then a small text that will be displayed, saying what's wrong with this. So doing that with betterer, it's very easy. So, you know, we had rules, for example, for removing some migration comments, we had some import things we had, like class components, that kind of stuff, adding this as a rule to betterer was two lines for each of those rules, literally, and one of the lines was, you know, the regex, the other line was, replace this with that, or we try to not use class components

Jon Allured: 8:27

The warning message.

Pavlos Vinieratos: 8:29

Exactly. But then for and then the rule is kind of doing it by itself. So I don't remember the name, it's kind of like, betterertestrule or something like that, that you just cannot use as a function. And then you give it these arguments, and it's done. It's doing the work for you. Whereas danger, it was basically just running JavaScript, right? So we had to find all the files, and then do like a for each or something through all of them, and then do the regex ourselves, and then get the match and then replace it. And then whatever, whatever. So it was all very manual, which I'm sure you know, if we wanted, we could export it into like a little function, or something and make it look as clean as betterer. But then I thought, you know, if this tool exists, like we might as well just use it.

Jon Allured: 9:21

Something I find interesting about betterer is the ideas around the ideas that it has around, like incrementally getting your codebase better. So if I were listening to this episode, and I was working at a company that, for instance, didn't have like strict mode turned on for TypeScript. One thing you could do is like, configure betterer to see how many strict violations you have not allow it to go any higher than that number as of today, but then every time you get better, it keeps tightening up. You know, how many are allowed? Yeah, and so the energy over time, you can get yourself complying with strict mode and then turn it on. Do we have any opportunities like that, that you're seeing in in Eigen?

Pavlos Vinieratos: 10:11

Let me actually pull this up.

Jon Allured: 10:13

And then also, is it do you like check in the fight? Like, how does it persist? How many of each? Right? Yeah.

Pavlos Vinieratos: 10:21

So this is one thing that I definitely want to discuss, because it is the reason that after like, a week of usage of betterer in Eigen, we ended up removing it again. Which is something that yeah, we will definitely discuss, but to get to your previous question first. So I see here, I'm looking at the PR, which is also public, if people want to just check, you can, you know, find artsy/eigen on github, look for betterer in the PRs it's there. So we have, for example, something similar to what you're saying we had useanimatedvalue instead of usesharedvalue, right, It's just a reanimated thing. Very specific. We had some old code, it still works. But it's kind of, you know, we want to replace it. So you had a bunch of a bunch of those are, like 80 of them. So we thought, okay, no more from now on. And if someone removes it great. Little by little, as long as we don't have new ones. And other one was for replacing woman with luxon. We definitely had the strictness migration, like you said, because we went to like, strict mode two, three years ago, maybe, and then some of the code was not ready for it. So we, you know, we had some ignore comments, basically there. And we're trying to remove those slowly. Yeah, and a couple others, some really things, tests that, you know, we accidentally forget. And we like if you focus on a test, or if you X, like if you cross it off. And then sometimes we forget about this, right? So this helped us stay at zero, right, that kind of thing. And then last one was the class components here.

Jon Allured: 11:59

So you turn this on, and we live with it for a while. George, what was it like to have betterer or running on Eigen? And did you see any pain?

George Kartalis: 12:08

I personally loved it.

Jon Allured: 12:11

Nice!

George Kartalis: 12:11

In the beginning, Like, in the beginning, we had some issues, like for example, in the commit flow, but I gave some feedback to Pavlos on that. Like, for example, you were making a commit. And when you were pushing the commit, then the betterer like file was generated afterwards. So you had to do an extra commit in order to update it, basically. But then we we fixed that, that change.

Jon Allured: 12:40

So is that switching it from like a link to a before-commit hook? And then that hook has to add the file?

Pavlos Vinieratos: 12:46

Yeah, basically. So yeah. And I think you asked that before, like, how does it track how many of those exists, those like bad things that you want to remove? So

George Kartalis: 12:53

It's kind of the same way as detect-secrets yeah, it does have an extra file, like, you know, you can imagine or think about of it as lock file. So it has like a betterer.results. And it just keeps, you know, the rule that's violated and the line and the file, file line, like column or something like that, so it knows exactly where like the problem is, and then can show it to you and tell you, like this was added now, and these ones I know about. But yeah, like George said, if you add, you know, a new line or 10 new lines in a file that some violation already exists, then this results file is going to be updated, so that the new line in this file is correct. So you know, imagine you have like, the second line is your bad import, for example, in an import something above that, now my bad import is on the third line, right? So this needs to be kept up to date. And yeah, like George said, In the beginning, I totally forgot to have this as a pre-commit hook situation, you had to make another commit, not great. We fixed that. was working. Because like, you have to update their secrets baseline every time. But yep, the iteration. The first one, like didn't let us to update it.

Jon Allured: 14:15

What's that? What's that file? I'm just looking at the repo on GitHub.

Pavlos Vinieratos: 14:19

Again, it's in the PR, like #8045 is the PR, and it's the file called.betterer.results. So if you look at our file is basically just a long, long list of just, you know, arrays of file name. Yeah. And like I said, the line column and distance, I think, you know, like how long the error is, for example, and then the rule that's violating and the so we had, you know, that issue in the beginning and but that was not enough of a reason to remove this. This was more about like, let's just fix this to be better and friendlier. And then we had some other issues. uses that people complain about and one was. And this is kind of like a betterer choice, I guess. So it has this reporters, it's called, or they it's calling it, which is how it will display. You know, the results, like how many bad things are there? How many removed how many you added that kind of thing. And the default reporter is kind of noisy, like it has, you know, like a 10 line, logo, or like an ASCII logo of itself, which is great to look at, but like not great when it's on your CI and it's like messing around with all the stuff. It's clearing the, the, you know, your console, so you lose all the history, gets a little bit, you know, taking over your terminal, which will not great.

Jon Allured: 15:45

Where you sort of want like, thumbs up, thumbs down. Or something.

Pavlos Vinieratos: 15:50

Exactly.

George Kartalis: 15:51

Depends on their perspective, though. Because, like, I personally liked it, because it's like running like TDD approach or something. Because it gives you these like green checkmarks over time. And yeah,

Pavlos Vinieratos: 16:05

yeah, give your like, you know, whatever

George Kartalis: 16:08

the dopamine receptors are happy.

Jon Allured: 16:11

I love seeing green dots. Nice.

Pavlos Vinieratos: 16:13

So yeah, that was one of the things that And then, you know, I basically this stuff. people you know, so I guess some people really liked it, some people really didn't like it. But to be fair on CI, it is kind of a, it is noise, right? It's a little bit more than we could have there. So then the first thing we tried was I wrote a new reporter for this, which was surprisingly easy. Yeah. So I tried to do that. But he or the developer of betterer he has this react-ink, I think it's called, you know, where you use react to, to print stuff on the terminal with ASCII. So that was already kind of like too much, I just wanted some console logs, you know, so I didn't do that. But it did have a very nice API of like, you know, if there is an error, it gave me hooks to like, you know, this run failed, this run succeeded, this whole suite of tests succeeded or failed, and then it gives you all the info you need. So then I could just say, you know, if there's no change, do nothing, if things are good, then say, You removed two things well done. And if you added bad things, then I could get, I could print the line of code that, you know, the thing was added on with some extra padding on, you know, below and after, and then give the methods that we want. So it was pretty nice and clean. And that definitely helped. But then yeah, we had an extra issue, which I guess, George, you fell on that right, which is just too many file changes, or too many changes of that file? I guess?

George Kartalis: 17:57

I don't think I came across this. I think it was another person. I came across another similar issue on like, with the eslint upgrade that we did recently.

Pavlos Vinieratos: 18:08

Yeah, so much tooling, so little time.

Jon Allured: 18:12

Yeah, so let's talk about that. So someone when Eigen runs on CI, it does a few different kinds of jobs. All kinds of things like type checking, eslint, building images, whatever. So when betterer was added that mean that you updated the circle CI config to have a new job? For betterer? Yeah, okay.

Pavlos Vinieratos: 18:31

Yeah, exactly. Basically just took the place of the danger rule, because danger will still there, running. Now I put just a different command. But yeah, the reason that, You know, we all have these rules that we were checking are a lot, there are a lot of, let's say, violations, you know, these rules. So, the moment you touch pretty much any file or like, you know, every other file, you will have to update this file. So it will do this automatically with pre commits and all that stuff. But of course, you know, there are people and like, everyone does that sometimes you just commit no verify or like putting a verify because it's faster because like you know it because you did it already, like whatever. But that obviously caused issues. Like we have exactly the same thing with detect-secrets, but because the secrets file, like tracker file or whatever, you want to call it, comparatively, yeah, it's not happening that often. So even though we have exactly the same thing, the the fact that it's so spread, I guess, in our codebase makes it hard and makes it very probable that someone somewhere will have this issue. And it's not their fault. Like you know, we all skip, Sometimes these hooks and that's fine. As long as the CI it's easy to do this or fix this for us or whatever.

Jon Allured: 19:56

Yeah. So so so if we got closer to zero on some of these betterer violations, that would mean fewer folks would bump into betterer violations, causing their PR to, you know, whatever. So like, maybe that's the next step.

Pavlos Vinieratos: 20:15

yeah. I guess, you know, if we wanted to do that, we could also just remove a few rules or add them, one by one or something like that. But the whole idea of this tool is that you don't need to do this, you can just like throw all that stuff, we will measure all the numbers for you and give you the changes. So then, what ended up happening after we talked about this with a few people and like, try to see what the actual issue is and what the solution would be. And this might end up as a PR, to the betterer repo, but the thing that people wanted is not to have, let's say, this results file with every file, every column, every line, blah, blah, blah. And rather just have a number, like, you know, the tracker file can just be I have three of those violations, seven of those violations. And then, you know, if you add one, we don't know which one is the new one, but at least we know that it's on your branch, right. So then, at least we can, you know, we can handle that. So then we don't have this million files or million line file. And it could just be like seven lines, much easier and much.

Jon Allured: 21:27

More manageable.

Pavlos Vinieratos: 21:28

Yeah. So we'll see, you know, we'll we'll suggest this to, to the to the better, they've will, or we might do a PR, we'll see. But yeah, we just to minimize this whole thing. We ended up removing this again, in favor of danger.

Jon Allured: 21:43

And I mean, that's, that's a good spirit of like innovation and, and, you know, improving over time, which is really what betterer is about anyway. So that's, that's, that's cool. Yeah, so I think we've pretty well covered this. We we like betterer, we think it's got some advantages over some of our use of danger. We're still experimenting. Any final thoughts from you, George?

George Kartalis: 22:04

Yeah, one final thought about betterer that I love the way that it helps you keep track of the tech debt basically and give you the, like, make you happy when you cross out some of these, like, tech debt points that it saves and like, also to note that it's not going to replace danger once and for all, because we have other rules like conventional commit rules that are like org level and stuff like that. Which are different.

Jon Allured: 22:33

Yeah, definitely. There's a there's a time and a place for each of these tools.

Pavlos Vinieratos: 22:37

Yeah, it was fun to try.

Jon Allured: 22:39

And it's nice to find libraries that are easy to work with. Right?

Pavlos Vinieratos: 22:43

Yeah, for sure. And also, you know, the developer, like he has a discord. It's he's very reachable. It's, I've talked about some of that stuff, and I'll send him some more. It's so nice to have someone in the open source community that's very responsive and quick with a bunch of things. It's like it's hard to know, and it's good to see that.

Jon Allured: 23:07

okay, great. Well, thanks for joining me and discussing this topic. as things progress. Maybe we'll be able to update listeners on where we where we end up. But yeah, we're gonna always try to get.. betterer. Okay, thanks, everyone for listening.

Matt Dole: 23:28

Thanks for listening. You can follow the Artsy engineering team on Twitter at Artsy open source, and you can find our blog at artsy.github.io Hi, oh, this episode was mixed and edited by Jesse and Kanye, and our theme music is by Eve Essex, who you can find on all major streaming platforms. See you next time.