tomchor / Oceanostics.jl

Diagnostics for Oceananigans
https://tomchor.github.io/Oceanostics.jl/
MIT License
24 stars 8 forks source link

Should we change function names to match Julia's style guide? #168

Open tomchor opened 3 months ago

tomchor commented 3 months ago

The naming of functions in this code doesn't follow Julia's style guide. Namely, our user-facing diagnostic functions have capitalized camel-case names. I'm emphasizing user-facing because all the internal functions already follow the style guide. As an example:

https://github.com/tomchor/Oceanostics.jl/blob/f5fbff7333100f04e8f67bcca288d74e2562fbe8/src/TKEBudgetTerms.jl#L362-L370

While the guide recommends that functions be named with lowercase. So the above function would be

function kinetic_energy_forcing_term(model::NonhydrostaticModel)
    ...
end

This was brought up in https://github.com/tomchor/Oceanostics.jl/pull/164 and https://github.com/tomchor/Oceanostics.jl/pull/167 so I thought I'd open an issue to discuss it.

cc @glwagner @jbisits

tomchor commented 3 months ago

There are two reasons why things are coded this way. First, when I started coding the package I saw we were probably gonna use long verbose names clearly distinguish each diagnostic. So reducing function name length by avoiding underscores seemed like a good idea.

Second, and more importantly, I thought that camel case made code more readable. To get some examples from the docs, from the user perspective things currently look like this:

Q = QVelocityGradientTensorInvariant(model)
∫χᴰ = Integral(TracerVarianceDissipationRate(model, :b))
Ri = RichardsonNumber(model, add_background=true)
PV = ErtelPotentialVorticity(model, add_background=true)

According to Julia's style guide, we'd be using lowercase with underscores, making things look like

Q = q_velocity_gradient_tensor_invariant(model)
∫χᴰ = Integral(tracer_variance_dissipation_rate(model, :b))
Ri = richardson_number(model, add_background=true)
PV = ertel_potential_vorticity(model, add_background=true)

Of course this is a subjective point, but I think it's much easier to read the code the way things are currently written, especially since the keyword arguments are written in lowercase as well.

I'm curious about other points of view, but I don't think there's much advantage in changing the function name style for the users. I think the advantage of switching lies mainly when getting contributions from other people. When modifying something in the code, new developers currently might get a bit confused about what is a function and what is a type since they expect the style guide to be followed. Thus following the guide would potentially make contributions easier.

I also should mention that I think it's okay to deviate from the style guide (after all it is just a guide) if we think there's an advantage. In this case I originally thought making the final user's code more readable was advantage enough, but I'm happy to discuss.

jbisits commented 3 months ago

I agree with the points above. My main thought on this front is that Oceanostics.jl is used with Oceananigans.jl so using the same naming for the user-facing functions is appropriate.

glwagner commented 3 months ago

It's often helpful to know what is a type and what is not. Just from reading the code we should be able to assume that KineticEnergyForcingTerm is a type. When we learn that is it not a type (or alias), this can lead to confusion.

Oceananigans also has some inconsistencies, leftover from when we didn't realize just how useful this can be especially during big refactors.

As for aesthetics, I have some sympathy. But I think the ability to understand code by reading it is more important.