On (29/09/15 17:02), Nikolai Kondrashov wrote:
On 09/29/2015 04:58 PM, Michal Židek wrote:
On 09/29/2015 03:49 PM, Nikolai Kondrashov wrote:
On 09/29/2015 04:27 PM, Michal Židek wrote:
On 09/29/2015 03:20 PM, Nikolai Kondrashov wrote:
On 09/29/2015 04:03 PM, Lukas Slebodnik wrote:
On (29/09/15 15:47), Nikolai Kondrashov wrote: >Thank you, Lukas. This is a simple and effective fix. > >However, I would prefer that we keep the || on the end of the line, >rather >than the start of the next one. Simply because the rest of the CI >code does >that.
I tried to follow convention from other parts of CI script. However I could not find a multiline test. All tests in CI script are single line either "[[ sth ]]" or "[ sth ]". So I decided to use similar was as in C code.
Ah, I see.
>If you prefer to have it otherwise, please change it everywhere.
There's nothig to change or did I I overlook something? If you still prefer to have || on the end of the line then I can change it. Just let me know.
There are two cases of "||" at the end of the line in commands (not []/[[]] tests) in contrib/ci/run, plus there are two cases of "|" and one of "|&" in other places, which are not the same yet similar.
If it's all the same to you, I'd rather keep the convention.
Thank you.
Nick
IMO the || at the beginning is easier to spot (same as we use in C), so I would use it that way.
Personally, I consider such approach awkward and more confusing (and can argue endlessly :)), but if people prefer it this way, sure, let's change it.
Nick
So, I guess we came to conclusion that nothing needs to be changed in Lukas's patch, right? :D
Er, no, I don't mind which way we go, but we'd better be consistent, so please either change Lukas's patch, or all the other places :)
Or, Lukas, did you mean that "||" outside []/[[]] tests is different and separate and we should have these changed only?
yes, it's something different. It's hard to notice || outside of tests []/[[]].
But it's better to be consistent and || before next command outsde tests is more awkward and more confusing.
Updated patch is attached.
LS