[docs]Update code guidelines for readability of setting table-like structures

[DaveTBlake]

Propose an update to code guidelines to allow for initialization of table-like strutures to be vertical aligned using whitespace and exceed line limits for readability. Thus option to skip Clang formating on these rare ocassions.

In the end human judgement on what is readable can be better than any set of Clang format rules.
For example

xbmc/xbmc/dialogs/GUIDialogMediaFilter.cpp

Lines 45 to 79 in 669f1b2

	static const CGUIDialogMediaFilter::Filter filterList[] = {
	{ "movies", FieldTitle, 556, SettingType::String, "edit", "string", CDatabaseQueryRule::OPERATOR_CONTAINS },
	{ "movies", FieldRating, 563, SettingType::Number, "range", "number", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "movies", FieldUserRating, 38018, SettingType::Integer, "range", "integer", CDatabaseQueryRule::OPERATOR_BETWEEN },
	//{ "movies", FieldTime, 180, SettingType::Integer, "range", "time", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "movies", FieldInProgress, 575, SettingType::Integer, "toggle", "", CDatabaseQueryRule::OPERATOR_FALSE },
	{ "movies", FieldYear, 562, SettingType::Integer, "range", "integer", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "movies", FieldTag, 20459, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "movies", FieldGenre, 515, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "movies", FieldActor, 20337, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "movies", FieldDirector, 20339, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "movies", FieldStudio, 572, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "tvshows", FieldTitle, 556, SettingType::String, "edit", "string", CDatabaseQueryRule::OPERATOR_CONTAINS },
	//{ "tvshows", FieldTvShowStatus, 126, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "tvshows", FieldRating, 563, SettingType::Number, "range", "number", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "tvshows", FieldUserRating, 38018, SettingType::Integer, "range", "integer", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "tvshows", FieldInProgress, 575, SettingType::Integer, "toggle", "", CDatabaseQueryRule::OPERATOR_FALSE },
	{ "tvshows", FieldYear, 562, SettingType::Integer, "range", "integer", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "tvshows", FieldTag, 20459, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "tvshows", FieldGenre, 515, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "tvshows", FieldActor, 20337, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "tvshows", FieldDirector, 20339, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "tvshows", FieldStudio, 572, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "episodes", FieldTitle, 556, SettingType::String, "edit", "string", CDatabaseQueryRule::OPERATOR_CONTAINS },
	{ "episodes", FieldRating, 563, SettingType::Number, "range", "number", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "episodes", FieldUserRating, 38018, SettingType::Integer, "range", "integer", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "episodes", FieldAirDate, 20416, SettingType::Integer, "range", "date", CDatabaseQueryRule::OPERATOR_BETWEEN },
	{ "episodes", FieldInProgress, 575, SettingType::Integer, "toggle", "", CDatabaseQueryRule::OPERATOR_FALSE },
	{ "episodes", FieldActor, 20337, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "episodes", FieldDirector, 20339, SettingType::List, "list", "string", CDatabaseQueryRule::OPERATOR_EQUALS },
	{ "musicvideos", FieldTitle, 556, SettingType::String, "edit", "string", CDatabaseQueryRule::OPERATOR_CONTAINS },

is more reable than after reformatting with Clang

static const CGUIDialogMediaFilter::Filter filterList[] = {
    {"movies", FieldTitle, 556, SettingType::String, "edit", "string",
     CDatabaseQueryRule::OPERATOR_CONTAINS},
    {"movies", FieldRating, 563, SettingType::Number, "range", "number",
     CDatabaseQueryRule::OPERATOR_BETWEEN},
    {"movies", FieldUserRating, 38018, SettingType::Integer, "range", "integer",
     CDatabaseQueryRule::OPERATOR_BETWEEN},
 ... [over 64 lines]
}

Hence permit the use of use // clang format off ... // clang format on in these situations.

[phunkyfish]

Table like structures are one example of where vertical alignment is acceptable. What are the others? SQL blocks for instance would be another case.

I think this section should be explicit on where this is permitted.

[Rechi]

In general vertical alignment isn't allowed to prevent having to realign a whole block, because a longer value is added. What should be done in that case for such sections? Should the block be realigned if the longest value is removed?

The lines in the example are longer then the current maximum line length of 100 characters and I see no exception about the line length in the new section.

Actually reading the new section only tells one can add // clang-format off and // clang-format on, but the nothing tells that all the code guidelines (including maximum line length and no vertical alignment) do not apply for that section.

[yol]

In general vertical alignment isn't allowed to prevent having to realign a whole block, because a longer value is added. What should be done in that case for such sections? Should the block be realigned if the longest value is removed?

If a longer value is added, realign - no way around it. And in this special case, I think that the benefit of having this code be readable in general beats having to do one or two more git blame steps in case you have to trace back something. If the longest value is removed, just use good judgement. If it's 1 character difference, no need to reformat it. If it's 20, different story.

I think this section should be explicit on where this is permitted.

It doesn't hurt to have some examples, but I don't think it has to be exhaustive.

[DaveTBlake]

The lines in the example are longer then the current maximum line length of 100 characters and I see no exception about the line length in the new section.

... nothing tells that all the code guidelines (including maximum line length and no vertical alignment) do not apply for that section.

Fair point @Rechi I have re-drafted as a separate section and added guidance on the points you raised.

@phunkyfish I could not find any SQL I would apply this to as an example, but not saying that there isn't some somewhere. I really don't think we need to be too prescriptive in these guidelines, or try to cover everything explicitly.

Since we are going to apply our guidelines via Clang script, and at some point automatically update all the code base to a common standard, we need to be practical and allow for those rare situations where the code would be made less readable by such automatic formatting. Sometimes the human knows best!

[phunkyfish]

It was just the other example I could think of where you want to keep specific formatting.

Example:

xbmc/xbmc/pvr/PVRDatabase.cpp

Lines 86 to 105 in 647327b

	m_pDS->exec(
	"CREATE TABLE channels ("
	"idChannel integer primary key, "
	"iUniqueId integer, "
	"bIsRadio bool, "
	"bIsHidden bool, "
	"bIsUserSetIcon bool, "
	"bIsUserSetName bool, "
	"bIsLocked bool, "
	"sIconPath varchar(255), "
	"sChannelName varchar(64), "
	"bIsVirtual bool, "
	"bEPGEnabled bool, "
	"sEPGScraper varchar(32), "
	"iLastWatched integer, "
	"iClientId integer, " //! @todo use mapping table
	"idEpg integer, "
	"bHasArchive bool"
	")"
	);

SQL can be hard enough to decipher if complex so it's another case where vertical alignment can make it easier to read.

[DaveTBlake]

Ah, SQL tastes vary @phunkyfish as does how you view that SQL. In the RDBMS itself and in debug the SQL is a single string again and the whitespace does not help at all.
The Clang formatted version of those lines seems just as readable to me

m_pDS->exec("CREATE TABLE channels ("
              "idChannel            integer primary key, "
              "iUniqueId            integer, " 
              "bIsRadio             bool, " 
              "bIsHidden            bool, " 
              "bIsUserSetIcon       bool, " 
              "bIsUserSetName       bool, " 
  . . .
              "bHasArchive          bool" 
              ")");

But I don't know if some other whitespace checker would get upset.
Having said that I would not object to SQL being layed out with whitespace, nor would I enforce it.

[phunkyfish]

Hah, you know I didn’t even check it. I just assumed clang-format would do something funny with it!

[DaveTBlake]

You know I think we should let Clang formatter lose on the codebase as a test, and carefully inspect the end results for areas that need "protection" or changes to the format (if possible). I am totaly for having an automated tool that helps maintain a common code standard, just hesitant about when we apply such a change across the codebase, and believe we need to do it intelligently.

[enen92]

@DaveTBlake code guidelines are also generated from doxygen (using the same .md file). Can you compile the docs and check if the change doesn't break anything?

[DaveTBlake]

Can you compile the docs and check if the change doesn't break anything?

@enen92 I don't know how to do that?

[DaveTBlake]

@Rechi I have reworded again, I hope this is now sufficiently explicit.

[enen92]

You just need to install doxygen and run doxygen Doxyfile.doxy in docs/doxygen folder. An html page will be generated one level up (in docs). Code guidelines is an entry in the sidebar menu, see: https://codedocs.xyz/xbmc/xbmc/md_docs__c_o_d_e__g_u_i_d_e_l_i_n_e_s.html

[DaveTBlake]

@enen92 I can confirm that this change works correctly in the Doxygen generated docs, thanks for pointing out this extra check.

Anyone else want to comment, otherwise I will merge this tomorrow.