CodingStyle: add python and javascript/typescript

[jecluis]

As we are rapidly expanding our code base beyond C++ and C, it is a good
idea to enforce coding styles for other languages.

Keep in mind that the patch is already setting the stage for upcoming dashboard v2 related code, using Angular for its front-end.

Signed-off-by: Joao Eduardo Luis <joao@suse.de>

[jecluis]

Btw, I requested for a few reviewers that may have some thoughts. Surely more people will have something to say about this, and maybe some of those to whom reviews were requested may just go "meh" (which I somehow doubt very much ;) )

[alfredodeza]

This half-baked enforcement means that it will be difficult to detect if a change is compliant unless it is super obvious.

PEP8 is not super obvious all the time which is why most rely on a text-editor with a linter that shows these in real time.

This means we can't have an automated check because it will never pass from the get go. How could one possibly identify what is wrong with a proposed change if the errors are mixed with other parts?

In other Python projects we made the effort to clean up and then enforce.

[jecluis]

@alfredodeza any suggestions how we can make this work better then? Ideally, I would love to have some form of coding style guidelines for code to come (e.g., mgr modules), but would love even more to do this the right way :)

[jecluis]

@alfredodeza I'm guessing enforcing and forcing things to be cleaned up would then address your point? Any suggestion on a linter to enforce PEP8 (or something else)? (I'm not sure whether the depth - or lack thereof - of my knowledge on this topic is fully transpiring ;) )

[dmick]

I mean...this isn't any enforcement at all, and isn't trying to be; it's just a style README

AFAIK we don't enforce C++ or shell standards by and large

[alfredodeza]

What is the use of a "style README" that can't be verified, and therefore it can't be asked for correction? If a PR comes along with non-pep8 standards, then is it OK then to just merge because we don't really enforce it?

This is too ambiguous, we either want code in pep8 style or not. If we don't want to enforce it then I don't see the need of adding this to a README

[liewegas]

How painful would it be to make the existing code conform?

[jcsp]

While I would like a jenkins job to verify PEP8 as much as the next person, specifying it in our style guide is still useful without the verification -- it enables reviewers to ask contributors to get their code reasonably close to PEP8 before merging. Especially if we're planning to add enforcement in the future, it makes life easier if we communicate this to contributors now.

As for converting existing code -- most of the mgr code is fairly close to pep8 (i.e. it doesn't light up like crazy in pycharm), and I wouldn't mind accepting patches that take it the rest of the way there, once we've decided how strict we want to be. We should concentrate on the mgr code (where all the new stuff is happening) rather than the rest of pybind, where it probably matters less.

[liewegas]

👍

[jecluis]

So, it is reasonable to leave it as is, or would you like me to reword some of it?

[tchaikov]

What's the use of an opinion that's not a law?

IMHO, the benefit of having a guideline outweighs the "half-baked enforcement". we do have existing code not conforming to the C++ guideline we are enforcing (please allow me use this word =), but this does not prevent us from having a code style guideline, just because we don't have an automatic machinery to detect the inconsistencies, for instance, the trailing spaces in C++ source code.

i don't think we can fix all of them problem in this single PR.

• for enforcing the C++ coding style, we could have a .clang-format, and suggest developers to install a pre-commit hook.
• for enforcing the PEP8: we could use pep8/flake8 in the pre-commit hook also.

on the jenkins side, there is more work to do. assuming the levels of conforming to code style guideline can be compared, we could keep track of the current level (or code style conforming status) in master in a repo (could be a github repo), and only accept the PR which does not decrease the level of conformity.

yeah, all of these need more love and efforts. but we can do it by piecemeal.

[jcsp]

To alleviate the concerns about enforcement and be clear that this is an as-yet-unenforced rule, I suggest:

• changing "For all python code" to "For new python code"
• remove the "Existing code should not be converted" so that at the point anyone adds enforcement, they can bulk convert some files (this will be a necessary part of enforcement).

Does that work for everyone? I don't want to stay in a "no guideline at all" state just because we don't have the CI bits in place yet.

[LenzGr]

I agree to adding a clarification that this applies to newly written Python code.

As for changing existing code: I'd suggest to not explicitly discourage people from doing so, but modifications and improvements for PEP-8 conformity should not be mixed with other code changes / features in the same PR, at least these should be separate git commits.

I think it makes sense to definitely welcome contributions that take care of cleaning up code from a style perspective.

FWIW, this is how we addressed this in the openATTIC contributing guidelines:

• New Python code should adhere to the Style Guide for Python Code (PEP 8) with the exception of using 100 instead of 80 characters per line. Use the flake8 tool to verify that your changes don’t violate this style before committing your modifications.
• Existing code can be refactored to adhere to PEP 8, if you feel like it. However, such style changes must be committed separately from other code modifications, to ease the reviewing of such pull requests.

[dmick]

It's amazing that this needs this much discussion, but I agree vehemently with the last two comments

[sebastian-philipp]

-1 for enforcing a 79 char line length limitation. But that's just a personal opinion.

[liewegas]

@jecluis ping

[jecluis]

repushed, reworded as per @jcsp's suggestion, and added the following line inspired by @LenzGr:

Existing code can be refactored to adhere to PEP-8, and cleanups are welcome.

[jcsp]

👍