I’m a big fan of Boundary for managing dependencies between sections of code.
2 Likes
- consistent module layout (e.g. public funs are written before behaviour callbacks, are written before private funs)
- proper file naming (module
Foo.Bar.Bazshould reside inlib/foo/bar/baz.ex)- mandatory typespecs for public functions
- forbidding
alias Foo, as: Bar
@sasajuric Is there any possibility that you could post these somehow (e.g., as GitHub Gists)? I am setting up Credo on my personal projects, to figure out how to better use Credo at work, and I was surprised to find that the middle two aren’t included in Credo’s checks.
Others checks I have been looking for are mandatory typespecs on even private functions and mandatory documentation for types and public functions. I know I can write my own checks, but I’m just getting into Credo. So examples that I can directly use and then learn off of would be great. 
Some are inside the controversial section of the default .credo.exs
Mandatory typespecs:
{Credo.Check.Readability.Specs, []},
Some of the others that he mentioned might be there, but I’m not sure which is which.
I only know about typespecs because I use it.
Also make sure to give mix credo.gen.check some_check_name a spin to see what you can do.
2 Likes
As @crova mentioned, the spec check is included, and it even supports requiring specs for private functions (although I personally think this is counterproductive).
Likewise, the first and the third one are also present in credo ( Credo.Check.Readability.StrictModuleLayout and Credo.Check.Readability.AliasAs).
So the only one missing is the file path check. Unfortunately, I can’t share this code (it belongs to the client), and the reason why it wasn’t submitted upstream is because I wasn’t sure about the proper interface. The problem is that a typical Elixir project (e.g. generated via mix phx.new) violates this convention in a few places, for example with controllers and views (MyWeb.SomeController resides in my_web/controllers/some_controller.ex), and the code in test/support.
Another issue is that some teams choose to skip the top-level folder, i.e. by placing Foo.* directly under lib (instead of foo/lib), which I think is fine, but the check should support such scenarios.
I’ll think about this, and see if I can come up with some sensible proposal to bring this check into the credo project. I can’t commit on time though, so if someone else wants to start the discussion in the credo repo, feel free.
4 Likes
@sasajuric @crova Thank you both! I feel a little bit silly that I missed the typespec check, especially since that’s the one I was really looking for. I have been looking through the checks but simply missed these (will consider helping to improve the documentation of some of them). (Also, I noticed that there’s some issues the the VSCode ElixirLS plugin not displaying all of the documentation, particularly the example code in the docs.)
While I’m at it, the one other I’m missing is a check to enforce @typedoc and @doc on everything. I found the one for enforcing @moduledoc, so maybe I’m just missing these already existing.
and it even supports requiring specs for private functions (although I personally think this is counterproductive).
@sasajuric Do you mind expanding on this if you have time? I’m newish to Elixir and coming from statically typed languages like F#, I’m finding the number one reason code doesn’t work is that wrong or unexpected types are passed into functions. So, I’ve begun to add typespecs to everything for documentation purposes and for Dialyzer to try and help. I also will pattern match on empty structs, such as %Transaction{} = transaction, to help enforce some helpful errors when something wrong is passed in. This has the additional benefit that as the code evolves, it’s often helpful to throw in pattern matching of the struct’s fields in the already existing argument. With this approach, I’ve found it basically turns Elixir into as “statically” typed as possible, reducing errors.
I’m assuming the counterproductive argument is that it adds overhead to private functions or maybe that private functions should be so simple that they don’t require it? I’ve been finding in the code base at work, that there’s often a multitude of private functions that call each other. In reality, they probably belong in another module and made public so that they can be tested, but one approach I’ve found to improve the code base is to add typespecs to all of them (is basically the only way to figure out what they’re doing), and even add documentation via standard # <this is a comment> comments since they also don’t have docs. I still lament that Elixir doesn’t have @docp to help document private functions, which would display docs for them in IDEs like VSCode and could simply be ignored for documentation generation tools.
Anyway, interested to hear experienced thoughts on this!
1 Like
In my personal opinion, the main benefit of types is improved clarity of the code. As a reader, by reading the spec, I can learn more about the nature of the data coming into the function and out of it. However, too much of repeated information in a fairly small space can be counterproductive, and can in fact degrade the reading experience. Not to mention that having to spec every single function, together with adding pattern matches to simulate types, and documenting all private functions, is a lot of bureaucracy.
I once experienced a situation where a new lead developer insisted on documenting all the private functions. This had a couple negative consequences:
- A lot of useless noise which basically just repeated what was stated in the name of the function and its args.
- To circumvent this, devs started writing larger functions, which in fact affected the clarity in a negative way.
- People (me included) quit the job.
So the approach I use and advise for developing end projects (i.e. not a library) as a team effort is:
- Write specs for public functions (except functions marked with
@implor@doc false). - Don’t write specs in Phoenix controllers and plugs, where
@spec my_action(Plug.Conn.t, %{String.t => any}) :: Plug.Conn.tisn’t helpful, and is IMO just plain noise. Same thing for Absinthe resolvers, and any similar cases. - Specs for private funs are not mandatory, and should mostly be avoided. If the author and/or the reviewer feels they help, it’s ok adding them occasionally.
- Same thing for module/function docs.
- Using pattern matches to simulate types is discouraged. This is mostly bureaucratic noise which IMO degrades the reading experience.
7 Likes
Thank you for your thoughts! This is helpful.
I agree about the bureaucracy. It’s part of my initial transition into using a dynamic programming language at large. There’s a massive tradeoff working in a dynamic language, and in my personal projects, I have been countering that with this bureaucracy. However, I’m still playing around with these things in my personal projects to arrive at the right balance. A lot of typespeccing and documentation of private functions in my day job is to help make sense of the code, where the private functions are a little more complex than they need to be. It’s part of the transition to better named and smaller private functions.
I’m not entirely sure I’m remembering correctly, but I think I got the pattern matching on structs thing from Dave Thomas’ Elixir for Programmers course. Although, I’ve also seen it in some library code.
If Elixir could be gradually typed such that “functional core” modules could be type inferenced leaving dynamic types for “boundaries” such as messaging and I/O, what a world that would be!
Thank you again for your thoughts and the discussion. These are things I’ll keep in mind as I keep learning the Elixir ecosystem.
1 Like
I use it any like it. I find it necessary to generate the credo config file to make it clear that unwanted suggestions can take a hike though.
IIRC it doesn’t like TODO’s in general and I’m a huge proponent of adding notes about future improvements in the context of where they belong rather than filling up the backlog with them. The backlog will get pruned and deprioritized. My notes in code live forever in the versioning system for anybody who may work on that part of the code base in the future.
The IDE tools that can pull these out for review are incredibly helpful when working on an improvement/optimization focused iteration too. I’ve seen too many things buried in backlogs at this point and simply realized that notes closest to where they need to be used are more effective long term.
1 Like
Yeah, switching to a dynamic language takes some time getting used to. However, I don’t think that excessive simulation of typing is the solution. For me it’s usually enough if public functions are specced. Assuming the code is split into reasonably sized modules with well-defined responsibilities, I can usually quickly figure out the type of some variable/parameter in a private function.
I agree. Take a look at gradualizer and the Elixir wrapper called gradient (also check out this thread).
2 Likes
Thanks once again @sasajuric! I will take a look at those references.
Also, I might as well mention since I have you here in a discussion. It was your The Soul of Erlang and Elixir talk that cemented that I should learn Elixir and Erlang. I was already interested in it having done actor programming in other languages, but it really sold me due to the features of the BEAM VM regarding scheduling, which are unique to Erlang’s process (“actor”) model (as far as I can tell), and of course the variety of other features it has. Thanks.
3 Likes
credo --ignore Credo.Check.Design.TagTODO is how is part of my development work flow
Generate the config file and you can just remove that check from the warnings entirely.
2 Likes
I didn’t even know this option existed. I will do that then. Thank you for the tip.
