wire-server icon indicating copy to clipboard operation
wire-server copied to clipboard

Start coding style guide.

Open fisx opened this issue 2 years ago • 11 comments

I'm starting this in the hope that people will like it. It's a bit like ormolu: annoying at times, but it will free so much energy that currently goes into discussions about things that have been agreed on at some point in the past.

The rules I'm starting this with have been agreed on a long time ago, and I'm not sure what the current mood is, so I'm seeking feedback on two questions:

  • is a style guide a tolerable idea?
  • is any of the two rules i'm starting with acceptable?

I'm also curious if you're also eager to add something, but I'm perfectly happy to be the only one restricting our freedom in these ways. :)

fisx avatar Mar 10 '22 19:03 fisx

I don't have a strong opinion on the idea of having a coding style guide. So far I had almost no discussions with teammates about coding style, let alone disputes. For me it is not solving a problem that I have. On the other hand, if the guide is introduced, this means I will be expected to stick to it, meaning I will need more time and energy to write code. I agree this is a local view and if the guide is overall beneficial for the majority working on wire-server, then I will not oppose it.

If you will keep on working on the guide, I'd suggest to add a paragraph or two on the motivation and benefits of each rule. For example, why explicit or qualified import lists? What are its pros and cons?

mdimjasevic avatar Mar 11 '22 07:03 mdimjasevic

  1. All modules must have explicit export lists.

  2. All imports must be either explicit (import Data.Id (UserId)) or qualified (import qualified Data.Id).

I like 1, but I would like to argue against explicit import lists. The way I see it, it just adds a lot of extra work, noise and merge conflicts, for exactly zero benefits. What is the argument for explicit import lists, in fact? And what is the logic behind the prescription you suggest?

I would instead propose something in the opposite direction, along the lines of:

  • Modules export lists should be designed so that they can be imported unqualified most of the times. Try to avoid creating name clashes on purpose, and make exported names understandable without a prefix.
  • All imports should be implicit, except for modules that are intended to imported qualified. In case of clashes, prefer hiding to qualifying, and qualifying to explicit imports. Reserve explicit imports for modules that export many conflicting names and are awkward to use qualified (e.g. Control.Lens).

pcapriotti avatar Mar 16 '22 06:03 pcapriotti

Ok, no consensus in reach there. :)

I think I may be ok with adjusting to your rules @pcapriotti once I have HLS up and running. Currently I rely on the import lists to understand where names are defined, that's (probably?) where our disagreement comes from?

Having said that, I really dislike hiding, and I think I will keep disliking it. But I also dislike bike shedding. Dilemma!

fisx avatar Mar 29 '22 13:03 fisx

Ok, no consensus in reach there. :)

I think I may be ok with adjusting to your rules @pcapriotti once I have HLS up and running. Currently I rely on the import lists to understand where names are defined, that's (probably?) where our disagreement comes from?

Having said that, I really dislike hiding, and I think I will keep disliking it. But I also dislike bike shedding. Dilemma!

I don't use HLS either. I normally use hoogle to find definitions, and it works pretty well and fast (I have a labelled bookmark in firefox, so I can simply type z weirdFunction and get its location immediately).

I also don't like hiding much, so I try to use it sparingly. I think it's mostly useful when there are clashes in our own modules, so this can simply be fixed by not creating clashes!

If we were using some other language, I wouldn't be opposed to having more explicit import lists. The problems with haskell that make this basically unworkable are, IMHO:

  • importing qualified without an alias is unusable (and it's hard to keep the aliases consistent)
  • record fields and ADT constructors are in the global scope (so you end up having to import a trillion names just to use a couple of record types)
  • modules can't be nested, and imports cannot be scoped

I think the only solution is to accept that haskell's module system is a joke, and not try to extract too much mileage out of it.

pcapriotti avatar Mar 29 '22 13:03 pcapriotti

I don't mind having a rule to make all imports explicit or qualified. Since I use HLS I can ignore these huge import blocks of text anyways when reading. I would make exception though, e.g. Imports.

smatting avatar Mar 29 '22 13:03 smatting

Change my mind: The possible merge conflicts on the import lines are enough for me not to favor explicit lists.

smatting avatar May 06 '22 13:05 smatting

What is the argument for explicit import lists, in fact?

It prevents breakage when a symbol is added to a module. If you do unqualified imports with no import list, the PVP (https://pvp.haskell.org/#dependencies-in-cabal) specifically calls out this type of breakage.

I have a mild preference for explicit import lists, especially for packages that are outside our organization. I find it makes it easier to determine where an unfamiliar symbol is coming from. HLS is fine when it works, which is why the preference is mild these days, but it's still nice to be able navigate the source with noting but text tools.

stephen-smith avatar May 09 '22 19:05 stephen-smith

What is the argument for explicit import lists, in fact?

It prevents breakage when a symbol is added to a module. If you do unqualified imports with no import list, the PVP (https://pvp.haskell.org/#dependencies-in-cabal) specifically calls out this type of breakage.'

There's not much value in this. We are pinning dependencies, so such a breakage will only happen when we change the pins, and will result in a compile-time error which is trivial to fix.

I have a mild preference for explicit import lists, especially for packages that are outside our organization. I find it makes it easier to determine where an unfamiliar symbol is coming from. HLS is fine when it works, which is why the preference is mild these days, but it's still nice to be able navigate the source with noting but text tools.

I don't use HLS, and I don't buy this argument. In fact, it seems to me that people using fancy tools like HLS are more likely to be the ones arguing for explicit imports, because they are such a pain to manage manually. And to get this benefit you really have to use them consistently (including forbidding Foo(..) style imports), which becomes unreasonable pretty quickly.

We can navigate the source with grep if we stick to non-clashing names for identifiers. But even without this convention, grep works reasonably well. There's also hoogle (both the public instance and our internal one), which works great most of the times, as I described above.

pcapriotti avatar May 10 '22 07:05 pcapriotti

[explicit import lists] are such a pain to manage manually

This doesn't match my experience; GHC is quite capable of telling me exactly which symbols are missing / extra for many years. Though, I don't generally treat extra imports as a fatal error in my private projects, at least for local builds.

We can navigate the source with grep if we stick to non-clashing names for identifiers. But even without this convention, grep works reasonably well.

This doesn't match my experience with recursion-schemes and other public hackage packages. Oft-times the only reference to a symbol is a usage, and there are dozens of potential non-explicit module imports and several packages to try to pull down before you can even start a grep to find the actual definition. Jumping over to hoogle is certainly possible, but I did find it slower than following an explicit import list.

For stuff within our repository, yeah, grep -R, rg, or ag all work fine.

stephen-smith avatar May 10 '22 15:05 stephen-smith

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 10 '23 07:05 CLAassistant

Is this something we want to revisit?

elland avatar May 29 '23 08:05 elland

Let's add a section where we can optionally sign or names? Something like

We've read this style guide and are making an effort to follow and and maintain it. Signed: @smatting, ...

smatting avatar Mar 27 '24 09:03 smatting