Read the Google code review guide and think if A+ contribution documentation should take some ideas from there

[markkuriekkinen]

A+ style guides
https://apluslms.github.io/contribute/styleguides/

https://github.com/google/eng-practices/blob/master/review/reviewer/index.md

[skulonen]

I read the Google code review & style guides. Here are some notes regarding things that could perhaps be incorporated into the A+ guides.

---

General

The author of the CL should not include major style changes combined with other changes. It makes it hard to see what is being changed in the CL, makes merges and rollbacks more complex, and causes other problems. For example, if the author wants to reformat the whole file, have them send you just the reformatting as one CL, and then send another CL with their functional changes after that.

I think this is a good point and could be mentioned in the A+ contribution guide. There was recently a case where I had created some new functionality, and the files I had modified were inconsistent in their style. Markku suggested that I should make 2 commits: one for the new functionality, and then one for fixing the style issues.

Either way, encourage the author to file a bug and add a TODO for cleaning up existing code.

Related to the previous point. We could have instructions for what to do when you come across style issues in the codebase.

https://github.com/google/eng-practices/blob/master/review/developer/cl-descriptions.md

We could have instructions for writing a commit message. Mention the significance of the first line and the empty line after that, and where to put "Fixes #XXX".

---

Python (https://google.github.io/styleguide/pyguide.html)

• Chapter 2.13 (Properties): Perhaps we could have some rules regarding when to use properties and when regular methods? The models in the A+ codebase are a bit mixed. For example, LearningObject.is_submittable is a property but LearningObject.is_empty is not; CourseInstance.enrollment_start is a property but CourseInstance.is_enrollment_open is not.
• Chapter 2.21 (Type Annotated Code): The A+ codebase doesn't use type annotations, and I know that adding type annotations to such a large project is not trivial. However, perhaps it's something we could consider? I've had good experiences using type annotations in Python. It makes the codebase easier to understand for a newcomer, when a function's signature tells you which types it expects. It also improves the development experience, since editors/IDEs can offer context-dependent suggestions, and you can be more confident that the code works at runtime.
• Chapter 3.8.1 (Docstrings): Seems like docstrings are sometimes used in the A+ codebase and sometimes not (see e.g. course viewset classes vs exercise viewset classes). Perhaps the style guide should say when you should use docstrings.

---

Javascript (https://google.github.io/styleguide/jsguide.html)

• The A+ Javascript style guide is currently very sparse (3 bullet points). We could add some rules to it, e.g. which quotes to use for strings, commenting practices.
• Chapter 5 lists some dangerous features of Javascript, related to e.g. var, line continuations, this, semicolons. Rules for some of these could be mentioned in the A+ guide.
• JSDoc?

[markkuriekkinen]

Thanks, you had good points there!