Open J5lx opened 7 years ago
As for my own opinion, I can live with every single convention I marked as prevalent. However there are some cases where I’d still prefer a different convention:
else
.
Allman:
}
else
{
OTBS:
} else {
Much less verbose!
EnumName::memberName
for consistency with ClassName::memberName
and StructName.memberName
But as I said before, these are just really-nice-to-have to me and I can live without them if it is decided that way. As for the topics where I couldn’t determine a prevalent style:
&
/*
/*&
to the identifier. This is incredibly common for pointers in C (where they are omnipresent) and it helps with declaration lists where type *var1, *var2
yields to pointers as expected while type* var1, var2
confusingly results in one pointer and one non-pointerIn this case the second one is somewhat important to me, but I can live with other length limits.
Thank you for doing this! @J5lx
I've thought about it too but never got around it. So far i've just tried to stay consistent with whatever style each individual class has had, but having a defined coding style would make everything much easier.
As for my preferences:
I'm no nazi when it comes to coding style, as long as it's not too verbose :)
Since I started contributing code here, I attempted to be mindful of the coding styles. Certainly nothing is completely consistent here, but I came to almost identical conclusions on what conventions were most prevalent. A lot of the conventions are contrary to the way I prefer to do things, but it's trivial for me to add a few spaces or whatever as necessary. I definitely agree with your suggestion for 15-17. A length of 80 characters is fairly standard, but I wouldn't mind a bit more room to work with :wink: . I have no opinion on enum naming, and I don't think 5 is worth the effort of changing. I feel fairly strongly that all files should have a terminating newline (2ii). Looking forward to having official guidelines for all this!
Any update on this @J5lx? I've already started to apply some of the prevalent things we've agreed on in my newest PR and also made a config file for "Artistic Style" Formatting for Qt Creator, for those who use Beautifier.
For the most part I was interested in hearing from @chchwy before making any final decisions. But while I’m at it I might as well address some points right now.
prevalent, while ' i ' can be somewhat easier to read, it can be such a pain to keep track of.
To me, this reads a little like you prefer i because it is a pain. That seems a little weird, so could you confirm if this is (not) what you mean?
I feel fairly strongly that all files should have a terminating newline (2ii)
I agree completely.
I don't think 5 is worth the effort of changing.
Could you clarify what you mean by this, @scribblemaniac? Are you referring to switching to OTBS or are you referring to unifying?
Ah right, yes that makes sense.
(1) As I mentioned above, my editors auto convert from tabs to spaces, so if you guys prefer spaces then that's fine with me.
(3) whoops that a typo, I meant I prefer ii (no spaces). I've never personally liked to use the prevalent and I consider it an annoyance to add spaces around all parenthesis.
(8) I'm fine with a recommended (80) and soft limit (120).
Happy to see some guys raise this issue.
I won't be surprised the coding styles we prefer are different. You might notice that I followed a minimal coding style, which is a subset of my company's coding style so I don't need to configure my editor to 2 settings lol.
prevalent
prevalent
prevalent
prevalent
a brace always has its own line.
void func2()
{
if
{
}
else
{
}
}
prevalent
all ok for me
all ok for me
seems 80 and 120 are good
UTF8 without BOM (good for cross-platform)
prevalent
prevalent
prevalent
mMemberVariable
because my editor doesn't highlight members
&
is always next to the type type& a
;
*
is always next to the type type* a;
for me &
and *
are a part of type, not name.
same as previous
prevalent
No matter what the final coding style is, I'd suggest using a style that source files could be automatically formatted by a tool, like AStyle. So that we don't need to worry about the style of pull requests.
Okay so I took some time to look at this again. First of all, to address a few points:
[…] so I don't need to configure my editor to 2 settings lol.
Many editors allow loading pre-defined code styles per project. The editorconfig I mentioned previously is a file in the project root that is loaded by editors that support it to automatically adjust the code style settings according to the project. A Qt Creator plugin is available, more information and plugins are on the website. @CandyFace also mentioned a config he made for some other tool.
I'd suggest using a style that source files could be automatically formatted by a tool, like AStyle.
I agree that tooling for this is definitely helpful, ideal would be something that can be run locally and has both a check-only mode for control and a check-and-correct mode for comfort. I’m not familiar with AStyle so far.
Other than that, I compiled the preferences that have been voiced so far in a document on Google Sheets for a better overview. So far we have consent on just under half of the items. Here are the ones without consent so far (where * indicates that other styles are explicitly also okay):
Pass-by-reference in parameter lists (15), Pointers (16), Pointer pass-by-reference (17)
It is true that, logically, * and & are part of the type, but as I explained in https://github.com/pencil2d/pencil/issues/683#issuecomment-305795897 binding them to the type can sometimes lead to very confusing code, which is why I feel quite strongly about this.
Done:
In order to achieve consent on these items as well I suggest this:
To get this going, I've changed my stance on these: (1) prevalent (2) one newline (ii) (3) unchanged (4) (i) (5) unchanged (8) (i) (9) unchanged (14) (iii) (15-17) unchanged
I still feel very strongly about not having spaces and using a coding style that doesn't waste too much vertical nor horizontal space i.e. (OTBS)
I am okay with spaces after comma, before and after operators and outer brackets
Position clarification
Changed positions
myStruct->member
is unambiguous, member variables can still be referenced within struct as simply member
which is no different from classes. Plus there is an argument to be made for consistency.New options First off I'd like to add some options for function naming conventions. These aren't things that can be added to editor configurations, but they are still part of the code style and they also seem to be inconsistent so I think it's worth talking about.
get
prefix (ex. getColor()
, prevalent)
ii. No prefix (ex. color()
)get
prefix (ex. getPlaying()
)
ii. No prefix (ex. playing()
)
iii. is
prefix (ex. isPlaying()
, prevalent)
iv. getIs
prefix (ex. getIsPlaying
, currently not used)set
prefix (ex. setPlaying()
, prevalent)
ii. setIs
prefix (ex. setIsPlaying()
, currently not used)on<signalName>
(ex. onDurationChanged()
)
ii. No prefix (ex. durationChanged()
). Obviously this will have to be different from the signal name, or in a different class from the signal.While were having this talk about code style, we should really talk about what is by far the most important thing for code readability, documentation! The codebase for this project is sadly very lacking in documentation in my opinion. A while back I added Doxygen support for Pencil (https://scribblemaniac.github.io/pencil/docs/), however it never got merged upstream because its useless without at least some documentation of functions and classes. You can learn more about Doxygen here. Here are some options related to the styling of documentation (all of which are equally prevalent since there is no documentation 😥 ):
/**
...
*/
ii. Qt style
/*!
...
*/
iii. Three slashes
///
///
iv. Two slashes and an exclamation mark
//!
//!
v. Asterisk spam
/********************************************//**
...
***********************************************/
vi. Slash spam
/////////////////////////////////////////////////
/// ...
/////////////////////////////////////////////////
/**
* ...
*/
ii. Skip intermediate asterisks
/**
...
*/
\brief
ii. Autobrief, ends the brief description at the first '.' encountered. Use \brief
if you want more than one sentence.
iii. A ///
or //!
comment before the main comment block/** Description of var */
int var;
ii. Add comment after variable (using same style as 22 except with <
)
int var; /**< Description of var */
@<command>
(ex. @param
)
ii. \<command>
(ex \param
)/**
* @param[in] var String to print
*/
void print(string var);
ii. Inline comment
void print(string var /**< [in] String to print */);
Positions on new options
Thanks for the update, @scribblemaniac, I updated everything and also changed my own stance on 14 to iii, so we now also have consent on 9 and 14. Also thanks a lot for bringing up those other points which I totally didn’t think about, especially the documentation ones. Speaking of which, I think you shouldn’t wait too long with PR’ing that Doxyfile; it’s more motivating to work on something like documentation when there is some tangible result like a nicely browsable site. I added all your points to the OP for a better overview. As for my own positions on those points, I mostly agree with you:
There are actually a number of doc options there that I have never personally seen used in the wild: 24ii, 25iii and 28ii not at all and 23v and vi only when no documentation generator was used. As for styles, it seems there are a handful of C++ projects using Qt style and \command
but the Javadoc style with @command
seems to be the de-facto standard elsewhere including pretty much all other language communities (that don’t have their own home-baked format) (e.g. Java (duh), PHP, JavaScript, just to name a few popular ones). As a result, I’m much more used to the Javadoc format than to the Qt one, and I imagine this might also be true for other people.
@J5lx Thanks for the updates and providing your positions.
I think you shouldn’t wait too long with PR’ing that Doxyfile; it’s more motivating to work on something like documentation when there is some tangible result like a nicely browsable site
I agree, although there is still a bit of work to be done before I would consider it ready. I will bump it up on my todo list.
You indeed are raising a valid issue @scribblemaniac, documentation would be nice to have, especially for newcomers that doesn't get the codebase. What are you missing before you would consider it ready?
My stance on the new options are the same as yours.
What are you missing before you would consider it ready?
I'll go over all of the options again just to make sure they are all what I want them to be. I would like to modify the styling a bit more because it is ugly right now I my opinion, but I might just leave it if don't have enough time. Most importantly, I would like to set up some way of automatically regenerating the documentation on the gh-pages branch when the code on master is changed. Unfortunately I do no have very much experience with travis and all that so if someone would like to help me with that I would very much appreciate it.
In order to keep this already long discussion focused on code style, I created a separate issue about the documentation: #716.
I've noticed recently that you've been making some style changes @chchwy, specifically it seems you've changed your stance on 3. One space each (i) to no spaces.
am I correct in that assumption?
@CandyFace yes, it's correct. :)
I updated the spreadsheet and the summary post. I should really try and find the time to get this going again. Not to mention the exporter…
It's been a while now since any progress has been made here, hasn't it?
While browsing Krita source code some time ago, I bumped into their "HACKING" file. I suggest that we make a similar file, naming it STYLE or CODING_STYLE or something instead though to make it a bit more obvious. https://github.com/KDE/krita/blob/master/HACKING
If you all agree on something like this, I could try to put something together today based on our current agreed choices.
While i'm at it, because it doesn't seem like this will ever get finished if we don't make compromises, I'm changing my stance on (5) II -> I That means that (5) should be done too.
We still need to agree on pointer and reference position though.
Sounds like a good idea. I also updated the summary; it’s really just the pointer / reference position now where we don’t have consent and I’d also like to have @chchwy’s opinions on the documentation style (19-28) (or just a clear statement that it doesn’t matter, should that be the case).
Something that really stands out to me when working on Pencil’s code is that there is no universal code style. While there are certain conventions that are more prevalent than others, it seems that it is never universally consistent. Since I think that everyone will agree that any style is better than no style at all, I compiled a list of the various conventions in use so we can have a discussion as to what to use (I’m aware I might be opening Pandora’s box here). Once there is consent on the style to use, we can unify the style of the existing code and add an editorconfig to help new contributors stay consistent with it (Qt Creator plugin available at editorconfig/editorconfig-qtcreator). I’d happily volunteer to take care of the unification and the editorconfig myself.
Ordering is arbitrary, numbers are only there to make it easier to refer to a particular item if necessary.
( example )
(currently prevalent)(example)
type& param
type ¶m
type & param
(quite rare, almost didn’t notice it because it looks so much like bitwise and)type *identifier
,type*
when no identifier is giventype* identifier
type * identifier
(quite rare)type *&identifier
get
prefix (e.g.getColor()
, currently prevalent)color()
)get
prefix (e.g.getPlaying()
)playing()
)is
prefix (e.g.isPlaying()
, currently prevalent)getIs
prefix (e.g.getIsPlaying()
, currently unused)set
prefix (e.g. setPlaying()`, currently prevalent)setIs
prefix (e.g. setIsPlaying(), currently unused)on<SignalName>
(e.g.onDurationChanged()
)durationChanged()
). Obviously this will have to be different from the signal name, or in a different class from the signal.\brief
\brief
if you want more than one sentence.///
or//!
comment before the main comment block<
, after comment:@
prefix (Javadoc style, e.g.@param
)\
prefix (e.g.\param
)@param
commandFor reference, there are a few things that do seem to be consistent throughout. For instance, line endings are always LF rather than CRLF, class case is always PascalCase, method case is always camelCase. I also didn’t include a few things that are almost universally consistent and should be a given, such as no whitespace before semicolons vs one space before semicolons.
If you think I made a mistake or forgot about / missed something that should be included, let me know. (Edit 2017-07-29: Added items suggested by @scribblemaniac)
Of course the prevalent styles might give a good direction, but I suggest we don’t take them as ultimate authority if we think there is a good reason to use another style, since someone (presumably me) is going to go over all the code to apply the style anyway, so we should take that opportunity.
Now, let the clash of beliefs begin. (But please do try to be flexible and keep it civil 🙂)