Standardize typespec definitions

[vanvoljg]

Currently, the typespecs are not consistent with each other. Sometimes they're declared with a parameter name, sometimes just the type. We should pick one style and try to stick to it, and probably before there are a bunch of people writing code for this. It will help lower the barrier to entry for newcomers.

For instance, I find it much more readable to have literal notation whenever possible. For example, ✔️[any()] instead of ❌list(any()), or ✔️{atom(), binary()} instead of ❌tuple(atom(), binary()), except for when the functional version is more readable or conveys better meaning, like ✔️map() instead of ❌%{optional(any) => any}. I also go for functional types over bare names (:heavy_check_mark:any() vs ❌any). (This last point is just me, so if anybody thinks it's extra visual noise, I'm just fine with bare names.)

Parameterized types should be reserved for when they add to typespec clarity, like when there are multiple identical types or when the return type directly uses a given parameter. Honestly, I like having lots of typespecs, so the case of multiple identical types should be abstracted to named types. (E.g. @spec foo(integer(), integer(), integer()) ➡️ @spec foo(hours(), minutes(), seconds()) or similar, where hours(), minutes(), and seconds() are defined types of integer().)

I like the way ExDoc styles the types, and the way the Erlang docs do their specs (usually). It's highly legible. If we followed mostly the same style, then it would make it a lot easier to jump from documentation to code. This is personal preference and I'm open to any other style, as long as it's fairly consistent. AFAIK, ExDoc normalizes things when it creates the docs, so the published versions are good, but consistency is nice for people editing and reading the code, too.

[doughsay]

Yes! We do indeed need a standard. Sorry I've been a bit fast-and-loose lately 😅

[vanvoljg]

No worries! You've been a furious coding monster. Seems like it was important for you to just get the ideas out there and worry about the smaller details later. We can work on this, eventually. I'm alright with standardizing things in "a little while" and then retroactively fixing the codebase.

[amclain]

We should pick one style and try to stick to it

Style guide time. 😁

A fast approach to this may be to take a community style guide and list amendments to it, like I do here for personal projects:
https://github.com/amclain/styleguides/blob/master/elixir/STYLEGUIDE.md

I find it much more readable to have literal notation whenever possible.

👍 Agreed

except for when the functional version is more readable or conveys better meaning, like map()

👍 Agreed

I also go for functional types over bare names: any()

My opinion: I try to avoid this because of the parenthesis noise.
Mix format's opinion: Add parenthesis everywhere!!!

For example, scan through this:

@spec process_stuff(batch_1 :: Batch.t(), batch_2 :: Batch.t(), opts :: [keyword()])
  :: {:ok, Batch.t()} | {:error, any()}

versus

@spec process_stuff(batch_1 :: Batch.t, batch_2 :: Batch.t, opts :: [keyword])
  :: {:ok, Batch.t} | {:error, any}

Now go back and scan them again and try to pay attention if your eyes anchor in certain places for a split second and don't want to move forward.

[amclain]

Here's a good video about style principles in general:
https://www.youtube.com/watch?v=ZsHMHukIlJY&feature=youtu.be&t=164

[amclain]

Since this is a community project, I should add another thought:
I'm all for holding the core contributors to a high level of standards. However, if someone is a newer or drive-by contributor, we should take that into account and err on the side of making it easy for them to contribute.

[doughsay]

Honestly I'm with you on the parenthesis noise and I hate that the formatter enforces it in some placed. I think we should remove parentheses where we can (i.e. on built-in or imported types) and for now we'll just have to suck it up for types referenced using a module... unless there's a way to tell the formatter to stop that?

[doughsay]

Here's a good video about style principles in general:
https://www.youtube.com/watch?v=ZsHMHukIlJY&feature=youtu.be&t=164

That video makes me want to put our line length limit to 80... I thought the default was that at one point? Did they change that at one point?

Also, let's get Credo in here!

[doughsay]

I was curious about enforcing line length and went looking. It turns out I think Elixir removed the documentation of the option and no longer advertises it's possible to set line_length in .formatter.exs. It still works, but there's no mention of it anywhere. Also, it turns out the internal "target line length" is 98... the formatter tries its best to be under that but can't always. No idea why the number is odd... ❓

As for getting rid of parens on tyeps: the problem is that the formatter doesn't actually even know the difference between typespec code vs regular code. It cannot know that Module.t is a type and not a function, and because it enforces parent on all function calls, it just had to add them...

[amclain]

it turns out the internal "target line length" is 98... the formatter tries its best to be under that but can't always

Interesting. I'm happy if we set the line length lower. 👍

I've got a vertical bar on my editor at 80 columns (and usually the editor window isn't much wider than that anyway). One social rule I've liked using is to try to target 80 columns as a soft limit, with about 100 being a hard limit. The way you described the formatter it sounds like we should just set it to 80 and sometimes it might go a little over. I'm all for moving forward with that.

the formatter doesn't actually even know the difference between typespec code vs regular code

I know. 😭 I don't expect we'll be able to be able to configure our way out of it; I expect we'll be stuck with parenthesis wherever the formatter wants them. I was just expressing my ideal formatting preference.