LibreELEC.tv
LibreELEC.tv copied to clipboard
LibreELEC
RFC: initial coding standards
This is for discussion!
I've created the initial coding "standards" list based on my personal interpretation of how the code is currently written, and should be written. It is an incomplete list. It may also be wrong.
Any existing standard in the list is open for change (for instance, we could decide to use source instead of ., or always use braces etc.).
We can change existing code only once we agree what the coding standard for the project is, or should be. It's surprising that we have survived this long without such a document.
Please discuss and suggest changes - if we can agree on changes (maybe use thumbsup/thumbsdown reactions to a suggestion?) then I will update the list.
Eventually we can merge this and new code should then conform to the "standard", and existing code can be updated (either en-mass or as it is changed - to be decided...).
@InuSasha thanks fixed.
Personally I'm not a huge fan of the "indent-within-comment" style, but it's used in a lot of the code so I've included it as a "standard" - if it's time to change it then thumbs up this comment (thumbs down to keep as-is).
Defining coding standards is pretty low on my priority list but I think if we define something we should write down how we want it to be not document the current status quo which some seem to be unhappy with (eg indenting)
I agree it's never been high on my list but in order to take the code base forward as a team I think we now need a documented list of basic project coding standards/style guide in order to avoid different personal preferences appearing in the code.
The entire purpose of this PR is to achieve a fairly simple list that does document how the code should be - I simply started with a list that reflects the majority of the current code base (as I see it) so that we can then add/remove/change items through discussion before merging this.
On line length, I need help understanding how config/options LINUX_DEPENDS (for example) is more understandable/maintainable as a single line of 187 characters instead of being split over 3 lines, the longest no greater than 83.
My general target for line length is no more than 80-100 characters, with exceptions for singular, but really long variable/path combinations, that just seem to stretch on forever.
On indenting, I'd like the indent-within-comment to be replaced with something more python-esque, where indent is determined by the "level" of the code within a codeblock. Initial entries have no indent. Something within an If-statement or for-loop gets indented. Comments are attached to the code and follow the indent of the code it's commenting on.
On braces/brackets/{}s, I use them purposefully when doing:
- string manipulation (required)
- default values (required)
- inserting a variable into a string to remove ambiguity (optional, but good practice imo)
As this covers most of my use of a variable, I do it the remainder of a time to be consistent as it caused no harm. It also helps with syntax highlighting in editors.
On line length, I need help understanding how config/options LINUX_DEPENDS (for example) is more understandable/maintainable as a single line of 187 characters instead of being split over 3 lines, the longest no greater than 83.
I typically use grep to find out where packages are being used, so for example to find what packages are using dbus I can run git grep PKG_DEPENDS_.*= | grep 'dbus[ "]' - I have it as an alias which produces more usable output:
neil@nm-linux:~/projects/pullrequest_repos/LibreELEC.tv$ whatuses dbus
dbus is used by:
packages/addons/addon-depends/chrome-depends/at-spi2-core/package.mk
packages/audio/pulseaudio/package.mk
packages/devel/dbus-glib/package.mk
packages/mediacenter/kodi/package.mk
packages/multimedia/SDL2/package.mk
packages/network/avahi/package.mk
packages/network/bluez/package.mk
packages/network/connman/package.mk
packages/network/wpa_supplicant/package.mk
packages/python/system/dbus-python/package.mk
packages/wayland/weston/package.mk
So now if those long lines are split over multiple lines it becomes more difficult to quickly grep all packages - the only reliable way would be to source every package, which is much slower.
I really don't have a problem maintaining lines that are wider than my visible screen, or console - my GUI text editor has a horizontal scrollbar, and even vi copes well with long lines. Wrapping long lines just to fit an 80 column display is anachronistic, and is in some ways less useful than if they remain a single long line that is easier to parse with standard tools.
Making the code more readable for a human can, at times, be less beneficial when the code becomes harder to parse with a computer. I'm not arguing for long lines everywhere, but there are benefits to leaving some long lines. The ones that should remain long are (IMHO) LINUX_DEPENDS and PKG_DEPENDS_*.
In some add-ons the PKG_DEPENDS_* lines have been wrapped, which I don't think helps anything.
On braces/brackets/{}s, I use them purposefully when doing:
I totally agree with your list of reasons, and generally I prefer to use braces everywhere too but have to stop myself when submitting new code because the current "project style" is to not use braces unless they are required. I do this because it is better to be consistent with the main body of the code than introduce individual personal preferences (hence this PR, really!)
Since this is quite a simple issue to resolve let's get some feedback on the preferred brace style:
- :+1: for braces everywhere, ie.
echo "${FOO}" - :-1: for braces only when necessary, ie.
echo "$FOO"but alsoecho "${FOO//a/b}"
Initial entries have no indent. Something within an If-statement or for-loop gets indented. Comments are attached to the code and follow the indent of the code it's commenting on.
I agree with that, although I wouldn't be strict about the comment following the code - if no indent works better (makes the code more readable) then that should be fine as well. Although if the level of indentation becomes unreadable then maybe the code needs to be rewritten!
I'd like a vote on this change:
- :+1: for no top level indent, and comments should ideally be indented to follow code
- :-1: for current style which is to indent code within comments
Not sure if we should be too strict.
As for braces: personally I usually prefer to use them but I also don't mind having code using just "$FOO" - and sometimes I even write such code myself
There are a couple of more important things IMHO
- proper quoting when using variable expansion (in general, everything should be under double quotes). Sure, LE won't build ATM if you have spaces in filenames/directories because too many stuff isn't properly quoted but we should strive to do it right
- unneccessary use of githashes: For most of the stuff we should use tags, not githashes (at least, where available). Use githashes only if you really need to reference an unreleased snapshot or if the upstream doesn't have proper releases / tags. Yes, that's also about the mass-bump scripts that currently pick up unreleased snapshots (eg inputstream.rtmp, not to mention the retroplayer stuff) - it's OK during pre-alpha but when we hit alpha/beta we really need to make sure eg addon version X.Y.Z was built from version X.Y.Z, not Z plus some dev commits
On braces/brackets/{} I try to avoid them when speicfying paths because I think cp $PKG_DIR/scripts/fs-resize $INSTALL/usr/lib/libreelec looks more natural when using git diff compared to cp ${PKG_DIR}/scripts/fs-resize ${INSTALL}/usr/lib/libreelec (the file path is more important and using ${VAR} the variable is getting more attention at first glance)
proper quoting when using variable expansion
Agreed. I'll add something to that effect.
unneccessary use of githashes:
I'm not sure if this is really a coding/style issue, but more of a process/procedure issue - but yes, we should "prefer" tags to revs/snapshots (when available).
Use of [ test1 -o test2] versus [ test1 ] || [ test 2] ? same with -a and &&
@antonlacon thanks - added reference to [ test1 -o test2 ] versus [[ test1 ]] || [[ test2 ]].
Use of echo versus printf? I personally think of echo when I want to print text, but it seems to be an fairly even spread of what gets used.
printf has been used to signal the build stages (BUILD, INSTALL, GET etc.), mainly because it uses a variable level of indentation. The current indentation level (a multiple of BUILD_INDENT_SIZE, default 4) is stored in BUILD_INDENT and this is converted to a variable number of spaces with:
printf "%${BUILD_INDENT}c $(print_color CLR_BUILD "BUILD") $PACKAGE_NAME $(print_color CLR_TARGET "($TARGET)")\n" ' '>&$SILENT_OUT
The reasons for the use of printf elsewhere are less obvious - there's no obvious reason why echo couldn't have been used instead (maybe a future cleanup candidate![1])
So yes, I would agree that when outputting/printing we should use echo, unless we need any additional formatting that is supported by printf (and generally, other than the build stages, we don't).
-
git grep printf config scripts | grep -v BUILD_INDENT(http://ix.io/1qR6) - all can be replaced withecho- maybe something extra for #3078? :smile:
@antonlacon thanks - added reference to
[ test1 -o test2 ]versus[[ test1 ]] || [[ test2 ]].
I came across this: https://stackoverflow.com/questions/3184164/what-is-the-bash-test-command-evaluation-order when looking for test operator precedence in reviewing if I was missing something in the if test comment inPR3098. Of note is that bash handles the -a and -o operators differently from && and ||.
Of note is that if you have a test result like TRUE or FALSE, then whatever evaluation reaches FALSE will not be tested when using || (because the TRUE already determines the end result), but will be evaluated when using -o.
It may be a minor micro-optimization, but may be worth changing going forward considering some of the tests fire off subshells?
@antonlacon yes, the lack of short-circuiting with -a and -o could be an issue, and in fact will require that this logic in PR3098 is rewritten as:
- if [ -z "${PKG_GIT_CLONE_BRANCH}" -o $(git branch | grep "^\* ${PKG_GIT_CLONE_BRANCH}$" | wc -l) -eq 1 ]; then
+ if [[ -z "${PKG_GIT_CLONE_BRANCH}" ]] || [[ $(git branch | grep "^\* ${PKG_GIT_CLONE_BRANCH}$" | wc -l) -eq 1 ]]; then
I'll add a note about the lack of short circuit, in which case &&/|| can be used as necessary. Thanks.
@antonlacon I've updated the list to say that the built-in test synonym [] (ie. -a and -o) is the preferred syntax, and the extended test syntax ([[ ]]/&&/||) can be used when short-circuit evaluation is a consideration.
Can we define a common numbering scheme for patches e.g. https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/mediacenter/kodi/patches/README.patches ?
