I have just added Credo to my project and I really liked it. I would like to take it a step further and implement further checks via plugins but I’m struggling to find a list of them… so my questions for you all are:
Which Credo plugins are you using (if any)?
Is the community gravitating towards a common set of checks other than those included in Credo itself?
Adobe has some. I know there are others but none off the top of my head.
There are definitely a lot of style plugins that are up to personal taste and I think should stay that way, so I’d suggest looking through the disabled ones that come with Credo as there are some good ones! I particularly like the one that forces you to always pass the :async option to use ExUnit.Case. This way you can be relatively sure that whoever set it did so for a reason and didn’t absent-mindedly choose the default.
Just having something in place gets you 80% there, if not more. You get diminishing returns as you add more, the value it provides starts decreasing after a while, especially in big teams. Checks that are disabled/added because someone that left 3 years ago didn’t quite like it/or thought it was amazing at the time will haunt you later on.
Will do. I’ll take a close look at the disabled list and make sure I’m not missing some I find useful
Yeah, I guess I’m at the point where defining a style for the project is still quite easy, so I’m trying to see where to strike the balance… if there were some clear preferences across the community I would probably swing that way - if only for the sake of making the code more readable / standard… but to your point perhaps the default credo checks already gets us there
if there were some clear preferences across the community I would probably swing that way
This is precisely it, and I believe the default checks are the answer here. Since Credo is open source and there are TONS more eyes on those default checks, I favor that decision over any in house discussion/decision.
This results in the same conclusion as the elixir formater arrived to:
The consistency over every random project you can open on the internet makes them more readable and easier to understand, simply because everyone formats/writes code in a similar style.
Consistency, even within private projects, is the number one reason I care to use Credo, though the code suggestions are nice too since I don’t use any AI tools.
Big teams, period, is where the deminishing returns are If people are spending long periods of time arguing over what should go in Credo, or people are adding stuff without telling anyone, there are bigger problems at play. All that to say is that I generally agree the default is ideal, but for me the defaults are a little much and are just there to guard against juniors. Like:
Also stuff like “alias all the things” and “no single pipes” are things that are context-dependent AFAIC. Useful if you’re just letting anyone loose on your codebase, not for my own things.
I remember we have one custom to check if the migration version is a correct timestamp and if it’s not far in the future. The story behind it is that we had some people that liked to touch migration files by hand, without the generator, and at some point we have a migration with timestamp from the future, so every time you wanted to rollback your newly created migration, it rolled back that one instead.
Apart from that I sometimes advocate to have a check forbidding “full imports”. I used it as an example in this article. The check is not perfect IIRC, but bypassing it takes some dedication.
We also use checks from excellent_migrations, but lately I see many people disable them immediately in their migrations, so I guess we have to reevaluate.
I dislike the raw SQL check myself but the rest I actually like. IMO just discuss with the team. I always recommend the maximum amount of checks and then we just disable whatever ends up being annoying or giving too many false positives.
So for our code base we have a few situations where async is not possible (modification of application config) and we have the setup block raise. In other cases I have been debating just emitting a warning to annoy devs to make them async. In some cases, it just is not possible to make async. But overall, most can be.
That’s my point, that it forces you to specify either true or false. A lot of people just make it default to true in Conn.Case but I don’t really love that either and prefer to always see the explicitly given option.
EDIT: Maybe that’s what you’re saying… better to just default to true and you then it’s clear that when you specify false it was intentional?
Super useful article! Just bookmarked it for future reference. Thanks for sharing.
Seems like there are some useful checks there. Currently I’m working solo, so it’s a bit easier to achieve consensus (mostly just a handful of demons are awake in my head at a time ).
Ya, it’s definitely totally fine to do that. I just like the Credo rule as it’s the least surprising thing. When I worked on a codebase where async: true was the default, I was unaware of that fact for a bit so I would put it anyway. Not a huge deal, but it causes inconsistencies. It’s also possible (though unlikely) that someone could be writing a test that needs to not be async and they think it is. This is all very, very minor stuff and can be solved with good communication (though we all know that can go). In any event, being required to put it just takes anything like that right out of the equation.