search

CITCOM-project / causcumber

Cucumber driven causal inference for testing computational models.
1 stars 1 forks source link

Coding style / static analysis #15

Open bobturneruk opened 3 years ago

bobturneruk commented 3 years ago

Do we want to use black? https://pypi.org/project/black/ We can enforce this through continuous integration. I think it's a good idea but there's some upfront effort in ensuring the compliance of existing code.

Static analysis (e.g. with codacy.com) may also be a useful link in the CI pipeline.

jmafoster1 commented 3 years ago

What does Black actually do? I'm suspicious of anything that calls itself "uncompromising". That usually translates as "going to be annoying at some point" :P

Similar with static analysis: What does it do?

bobturneruk commented 3 years ago

Black reformats your code / tells you if you're not conforming e.g. functions that take 50 arguments, lines of code 1587 chrs long, stuff like this https://www.python.org/dev/peps/pep-0008/

Static analysis is anything that checks code without executing it. In a strongly typed language you can go a long way towards seeing if the code will run. Codacy identifies bad practise (where bad=what we agree it is) e.g. https://app.codacy.com/gh/Mesnage-Org/pgfinder/issues?categoryType=CodeStyle A lot of it looks pretty trivial, but it adds up.

In combination one gets clean, consistent code from n collaborators who all have different ideas of what's good.

AndrewC19 commented 3 years ago

I haven't heard of black before but I'm all for keeping a clean and consistent coding style across the repo. I guess this is something which is best to set-up early to minimise the amount of code we have to go back and refactor.

jmafoster1 commented 3 years ago

I'm for it as long as it's not too draconian. I get a lot of pep8 warnings about list comprehensions being too long, but I find it much easier to read a line that's, say, 94 characters long than one that's been broken in a contrived place so that no line in the entire program is more than 80 characters long. If you've got a for loop within a method within a class, 12 of those characters are mandatory indentation anyway. I agree that we can't go around Haskell-style writing the entire program in a single line, but I like a bit of flexibility there.

Static analysis seems like a decent idea, though, if we can configure it. I guess it plays the same sort of role as the Java compiler to stop us making stupid errors.

bobturneruk commented 3 years ago

Line length is like the one option in black.

AndrewC19 commented 3 years ago

We can always extend the character limit to something that we agree on.

bobturneruk commented 3 years ago

120 is a popular choice

AndrewC19 commented 3 years ago

I think that would be a good option. If we are writing list comprehensions which exceed 120 chars it means it probably shouldn't be a list comprehension!

jmafoster1 commented 3 years ago

That works. I'm not sure I've ever gone over 120 in Python. Isabelle, yes, but that's another story...