Skip to content

Conversation

@tlunet
Copy link
Member

@tlunet tlunet commented Jan 15, 2023

This is a first draft for a contribution guide (that can be see as a first proposal), with in particular :

  • some guidelines for branching
  • basic documentation for black and flakeheaven
  • a first proposal for naming convention

Of course, some points in the contribution guide can be put to debate (in particular, for the naming convention)

@tlunet
Copy link
Member Author

tlunet commented Jan 15, 2023

An argument (slightly in favor) for camelCase

First quick definitions

  • PascalCase : VariableNameLikeThis
  • camelCase : variableNameLikeThis
  • snake_case : variable_name_like_this

Abstract

Even if PEP8 suggests the use of snake_case for variable and function names, the alternative use of camelCase may be better from a practical point of view. This would not be the first time we break PEP8 guidelines (for instance, max line length to 120 in current settings), but there are other arguments.

Uniform and simplified variable names

  • using snake_case allows the following equivalent naming without explicitly ranking those :
tend = 1.0
t_end = 1.0
Tend = 1.0

using camelCase gives only one candidate tEnd that is simple to read for most people, and would stay consistent with any other time variable naming.

  • some variable indicating a number cannot have a short writing in snake_case, for instance num_nodes (instead of nnodes), while others use the short format (e.g nvars). Using camelCase would uniformize the notation for both, while still being clear and concise : nNodes, nVars.

  • mix with acronyms are not very explicit in snake_case. For instance, mssdc_jac may be clearer using camelCase and putting acronym at the end : MSSDCJacobi. Eventually, one could consider adding underscore to camelCase when it helps separating acronyms, for instance : useJacobian_MSSDC (sidenote : that's what I thought mssdc_jac was when reading it first 😅).

The main point here is that for the applications we are considering (with a lot of n[blabla] and other similar parameters), camelCase is probably more efficient and consistent on the long run. And there is for now rare (if none) situations where variable names in pySDC are harder to read in camelCase than in snake_case (for instance, ìsIllconditionned or similar name should probably not be used).

Reading effectiveness and current trend

camelCase is actually used in Python core libraries (for instance, logging) and there is no urge of complying to PEP8 (on the contrary, see https://lwn.net/Articles/877115/). Also, a scientific study comparing both snake_case and camelCase support the idea that, even if snake_case is easier to read in general, camelCase

"leads to higher accuracy among all subjects regardless of training, and those trained in camel casing are able to recognize identifiers in the camel case style faster than identifiers in the underscore style".

So yes, using camelCase would require a bit of time for people using snake_case to get use to, but not necessarily long. Furthermore, the use of PascalCase for class names is already quite natural in Python, so it won't be a huge gap to go to object methods and attributes written with camelCase. Furthermore, if used wisely (with eventual help of underscore), it could help reducing the length of a code, so it looks more appealing for a user that don't know already how it works. For instance :

# instantiate controllers in snake_case
controller_mssdc_jac = controller_nonMPI(
    num_procs=num_proc, controller_params=controller_params_jac, description=description_mssdc
)
# instantiate controllers in camelCase
controllerMSSDCJacobi = controllerNonMPI(
    nProc=nProc, controllerParams=controllerParamsJacobi, description=descrMSSDC
)
# Note that removing underscore allows a more explicit naming for controllerMSSDCJacobi without too much more space ...

From a short poll I did in the math section in TUHH, 5 persons out of 6 that answered where more in favor of camelCase than snake_case (the only person favoring snake_case beeing @brownbaerchen). I know this is a bit bias, and not very representative, but more detailed research indicate that camelCase is slowly becoming a serious alternative to snake_case, in particular with programming languages like TimeScript (next generation JavaScript) that massively use it (Python almost getting there with PascalCase for class names).

Work will have to be done anyway

Currently, pySDC use a mix between snake_case and camelCase, with majority for snake_case. Going to snake_case would implies a lot of changes in all naming. However, this will have to be done already for man class names, and with the whole default parameter rework for v6. So this would be the perfect opportunity to refactor most of pySDC names (let the change be a full one 😄). And also, many IDEs and editors offer some very simple ways to change a given variable name now ...

So what do you think @pancetta ?

@tlunet tlunet mentioned this pull request Jan 15, 2023
@pancetta
Copy link
Member

This is a first draft for a contribution guide (that can be see as a first proposal), with in particular :

  • some guidelines for branching
  • basic documentation for black and flakeheaven
  • a first proposal for naming convention

Of course, some points in the contribution guide can be put to debate (in particular, for the naming convention)

I really appreciate your contribution, thanks s lot. Before you write more markdown files, though: does that play along with the ReStructuredText files used so far? Esp. for the website?

@pancetta
Copy link
Member

I’m totally fine with camelCase!

@tlunet
Copy link
Member Author

tlunet commented Jan 15, 2023

This is a first draft for a contribution guide (that can be see as a first proposal), with in particular :

  • some guidelines for branching
  • basic documentation for black and flakeheaven
  • a first proposal for naming convention

Of course, some points in the contribution guide can be put to debate (in particular, for the naming convention)

I really appreciate your contribution, thanks s lot. Before you write more markdown files, though: does that play along with the ReStructuredText files used so far? Esp. for the website?

That's an answer I don't have the courage to look for on a Sunday evening 😅

@pancetta
Copy link
Member

My 2 cents: I'm ok with having markdown files in the docs, as long as they're clearly targeted toward Github users/developers. My original thought was that I don't want to write documentation twice (website + Github), but in order to get things done, we could just have rst files where necessary and possible and md files for the lazy.. I mean.. where it's not straightforward.

@tlunet
Copy link
Member Author

tlunet commented Jan 16, 2023

grafik

Do I see here some love-hate relationship with camelCase @brownbaerchen 😄 ?

@brownbaerchen
Copy link
Contributor

grafik

Do I see here some love-hate relationship with camelCase @brownbaerchen 😄 ?

Well, the hate is directed towards camel case, but the love was meant for you ;)

@tlunet
Copy link
Member Author

tlunet commented Jan 16, 2023

My 2 cents: I'm ok with having markdown files in the docs, as long as they're clearly targeted toward Github users/developers. My original thought was that I don't want to write documentation twice (website + Github), but in order to get things done, we could just have rst files where necessary and possible and md files for the lazy.. I mean.. where it's not straightforward.

Then let's do this for the contribution guide : host it on github with markdown files, and put a full link in the README.rst such that it will be integrated as well in the documentation website.

@brownbaerchen
Copy link
Contributor

That makes sense. Since the contributing happens on GitHub, it should be prioritised for the corresponding documentation imo. I was really hoping you would find a long term solution for documentation of projects both on GitHub and on the website, though. :D

@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

Base: 68.47% // Head: 68.50% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (8d7cbfc) compared to base (eadf81c).
Patch has no changes to coverable lines.

❗ Current head 8d7cbfc differs from pull request most recent head 5101a85. Consider uploading reports for the commit 5101a85 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   68.47%   68.50%   +0.02%     
==========================================
  Files         226      226              
  Lines       19510    19510              
==========================================
+ Hits        13360    13365       +5     
+ Misses       6150     6145       -5     
Impacted Files Coverage Δ
pySDC/projects/soft_failure/generate_statistics.py 75.37% <0.00%> (+0.50%) ⬆️
...mentations/problem_classes/Van_der_Pol_implicit.py 98.36% <0.00%> (+1.63%) ⬆️
...C/projects/soft_failure/implicit_sweeper_faults.py 96.52% <0.00%> (+2.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pancetta
Copy link
Member

@tlunet Can you please merge the changes from master? Looks like codecov woke up..

@tlunet
Copy link
Member Author

tlunet commented Jan 16, 2023

That makes sense. Since the contributing happens on GitHub, it should be prioritised for the corresponding documentation imo. I was really hoping you would find a long term solution for documentation of projects both on GitHub and on the website, though. :D

Created #250 for that and an associated milestone 😃

@tlunet
Copy link
Member Author

tlunet commented Jan 17, 2023

I've messed up thing very bad (accidentally merged v6 into contrib through an other branch ...). I've restarted a new contrib-2 branch from start using cherry-picks, and opened a new PR #257.

@tlunet tlunet closed this Jan 17, 2023
@tlunet tlunet deleted the contrib branch January 17, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants