Servo style guide?

[xiaochengh]

(Context: https://github.com/servo/servo/pull/39052/files#r2333120334)

What's the style guide for Servo? I assume we are at least following the Rust Style Guide, but it doesn't cover what we are discussing at https://github.com/servo/servo/pull/39052/files#r2333120334.

Should we compile a more detailed style guide?

[simonwuelker]

In https://github.com/servo/servo/pull/39052/files#r2334241232 @TimvdLippe writes:

The reason I changed it is that in other reviews, I received comments that floating functions outside of impl is not desirable. Therefore, these functions should be part of an impl.

Free functions are perfectly fine in my opinion when a function doesn't logically belong to any one struct. Note that in the spec, all algorithms are floating functions. It surprises me that such comments were made at all.

As for compiling a more detailed style guide in general:
I believe ./mach fmt is sufficient to enforce a baseline code style that all PRs have to follow.
There are a couple other code-style issues like the one above - shadowing variables and using helper traits to extend items from different crates come to mind. I think different maintainers have different preferences here.
These issues can't easily be automatically verified, so enforcing a specific style here would put an additional burden on reviewers. Not to mention that it's more documentation that a first-time contributor would have to read.
As long as the code works and is maintainable I personally am fine with different styles in the same codebase.

(Circling back to the original issue, I argue that putting a method in a completely unrelated impl block is unintuitive, therefore not maintainable. So either a better impl should be found or it should be a free function.)

[sagudev]

I also agree that free standing functions are fine (in the right context of course). Having no freestanding functions is more of a object oriented pattern.

Note that in the spec, all algorithms are floating functions.

Yes, but some cases are better as methods taking &self.

helper traits to extend items from different crates come to mind

This also very much depends on context. I know we have https://github.com/servo/servo/blob/965e87d006ee4ac64407128e0c3d1fa104917294/components/script_bindings/utils.rs#L562 which should be removed IMO, but sometimes helper traits are needed unfortunately (especially after separating script_bindings).

[sagudev]

I know we talked about https://github.com/servo/servo/wiki/Code-Review being too relaxed somewhere, but still:

You don’t get to demand that the reviewee re-write the patch in your preferred style. If their code meets style guidelines, fixes the bug, and is efficient enough, don’t waste people’s time by re-architecting it.

[mrobinson]

I believe that it should be the job of a reviewer to ensure that code is maintainable. This can sometimes mean reasonable comments about the architecture of the code. The DOM is object-oriented and having methods implemented in related impl blocks makes code easier to understand and maintain. When there are many free functions it is a good time to think about whether there is a unifying concept that encapsulates their data and also their behavior.

That said, I agree that this rule shouldn't be taken to an extreme. Free functions are obviously preferable to putting methods on unrelated data structures and also reviewers should not be unreasonable in asking code contributors to consider the design of their code or ask for minor changes that could be considered improvements. I understand that opinions can differ about this -- I do not know if there is consensus among reviewers in the Servo community one way or another.

[xiaochengh]

Thanks for the discussion (and sorry for the delay).

I agree that it's probably too early for us to compile our own style guide. I'll refer to the Rust style guide for now.

Btw I was expecting Google to have a Rust style guide, but there isn't.