-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply a consistent code style #683
Comments
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:
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:
In 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. 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 😉 . 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.
|
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.
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:
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 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):
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: 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 New options
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 😥 ):
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 |
@J5lx Thanks for the updates and providing your positions.
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. |
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. 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 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.
(sometimes also in other kinds of brackets)
( example )
(currently prevalent)(example)
(sometimes also before other kinds of brackets)
(For obvious reasons, I have no idea if there ever was a line length limit in Pencil’s code so I’m only including the “none” option here which is easy to spot)
(apparently no prevalent style here)
type& param
type ¶m
type & param
(quite rare, almost didn’t notice it because it looks so much like bitwise and)(apparently no prevalent style here)
type *identifier
,type*
when no identifier is giventype* identifier
type * identifier
(quite rare)(this is currently not in master but I need it for my lav exporter so I’m including the choice that I made for discussion)
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.Only effective if i, ii, or v is chosen in previous item (23).
\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 🙂)
The text was updated successfully, but these errors were encountered: