Discussion on comment headers / sections to help split up the code

[JoeZiminski]

For longer modules it is common to add comment sections to help readability. For example in SpikeInterface you might have:

# LOW-LEVEL IMPLEMENTATIONS
def compute_correlograms_numpy(sorting, window_size, bin_size):

In the SpikeInterface convention (as above) is a comment above the function that starts the section. Sometimes I find these a little hard to see where sections start and end and was wondering if there is any appetite for using a different convention (some examples below).

Personally I find such headers make navigating the code easier but I think they can be a little contentious and I don't have particularly strong feelings either way, but thought I'd start a discussion!

(1)

# ------------------------------------------------------
# Low Level Implementations
# ------------------------------------------------------

(2)

# Low Level Implementations
# ------------------------------------------------------

(3)

#########################################
# Low Level Implementations
#########################################

[alejoe91]

Thanks for bringing this up @JoeZiminski

I think we should go with better headers for sure! Personally, I like this:

#########################################
# Low Level Implementations
#########################################

[h-mayorquin]

While this is an improvement with very low cost. If many of those are needed is probably indicative that a new file / module is required instead. That is, the module contains probably too many things if this occurs.

The (really small!) cost is:

• Having to thing on where to put your function (maybe not bad) and the arguments that arise from it (coordination cost).
• Small maintenance cost to keep them tidy.

[JoeZiminski]

Thanks both! I agree @h-mayorquin that a good end goal is to not have these at all. I think short-term these can be useful as indicators of where things could be factored out, and simplify future refactorings?

[h-mayorquin]

Yes, that makes sense to me.

[samuelgarcia]

I prefer sober stuff

# Low Level Implementations
# -------------------------

[JoeZiminski]

I don't have a strong feeling but have a preference for lighter edges that for me attract less attention. Because in the UK have another first-past-the-post election coming up and I am jealous of the PR system I will propose a PR style vote (I have numbered above) with alternative vote rules. My vote in order of preference would be [1, 2, 3]. However, if people don't agree this is a good approach we could have a vote on the best approach to deciding 😆

[zm711]

This what Sam did in Neo. I like the hashes because they provide a nicer contrast.

##########################
# Low Level Implementation

So I would choose this style followed by 3, 1, 2 :)

EDIT: accidentally linked a PR. Removed the link. haha.

[chrishalcrow]

I also vote 1, 2, 3. There's something about hashtags that hurt my eyes.

[alejoe91]

I vote for 3, 1, 2, @h-mayorquin @yger your votes are needed for democracy!

[h-mayorquin]

Let's keep the matlab tradition 3, 1, 2 as well.

[JoeZiminski]

@yger do you have any opinions on this critical topic 😄?

[yger]

I vote for 3,1,2 !

[JoeZiminski]

@alejoe91 @h-mayorquin @samuelgarcia @zm711 @chrishalcrow @yger

The votes are tallied and #3 wins!

#########################################
# Low Level Implementations
#########################################

I don't have the stomach to make a PR updating all of the existing ones, but I guess if you see any of these and want to update them, or add new ones in, this is the style to use. I'm gonna add a issue on various dev doc updates so will include this.

The other question is how long they should be 😅 I like 79 (PEP) or 88 (black default) for these, but just as long as the text also works (although the size will not be uniform). I also don't have the stomach for deciding this now either, so maybe we can free-for-all it for now and fix the lengths later!

[h-mayorquin]

Let's add this to the dev documentation and consider this issue close. Decisions are tiresome and we can implement it piece wise.

Thanks for leading this @JoeZiminski .

[h-mayorquin]

Here: https://github.com/SpikeInterface/spikeinterface/pull/3402