Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[konsolebox]

On Wed, Nov 30, 2016 at 8:35 PM, Mike Gilbert  wrote:
> On Wed, Nov 30, 2016 at 7:11 AM, konsolebox  wrote:
 I also prefer some things this way:

 - Indent the contents of the first `if` block for consistency's sake,
 and less confusion.
>>>
>>> I disagree; indenting the entire eclass is silly and does not really
>>> improve readability. Also, this is a very common pattern found in
>>> other eclasses.
>>
>> I prefer following consistency before anything else.  And it's just
>> uncommon and odd, but it's not silly.  Imagine if you use another `if`
>> block on the second level where more functions are defined.  Would you
>> also not indent that?
>
> That first "if" is a bit of a special case; it's only there to prevent
> the code from being executed more than once in global scope. This
> provides a minor speed boost when the eclass gets sourced more than
> once, and makes sure that any global variables are only set once.

OT: inherit() should have already been able to have a non-hacky
solution of making sure that eclass files are loaded only once.  Bash
4.2 has already supported global declaration of variables with -g (in
case the flag variable would need to be declared inside a function),
and associative arrays with -A.  Even older versions of Bash (<4.0)
can make use of multiple variables with a common prefix to imitate
associative arrays if compatibility is needed.  It could just check
BASH_VERSINFO if it has to downgrade to a less-efficient method.

Besides no longer having to rely on a special `if` block, it probably
would also make loading of ebuilds faster since script codes that are
referenced multiple times wouldn't have to be re-parsed again and
again by bash.  It's scary to imagine how it happens in all ebuilds
that's read in portage, but maybe repeated loading of eclass files
doesn't always happen, who knows.  (I think it's still significant
even if the dependencies are already cached.)

> I think of it as being similar to pre-processor statement you would
> find in a C header file; one does not generally indent all the code
> within a C header file, because it just introduces a big chunk of
> whitespace that does not improve code readability.

I did as well, but I'll still prefer to have it indented, because it
avoids trying to know what the last lone `fi` is for, if you haven't
seen the first `if` condition yet.  If they are indented, your mind is
automatically set to know that they are within a conditional block,
and no surprise happens.  Maybe that's not the case if people are
already used to seeing those setup, like in eclass files, but I'm not,
so it does help improve readability in my case.

To be honest I find it painful to see anything inside a block to be
not indented, no matter what the reason is.  You can say that they
look like preprocessors in C, and preprocessors don't make codes
indent, but preprocessors are not (functional) codes, and they are
naturally recognized to have their own "indentation space" or
patterns, unlike the shell's `if`.

But anyway, I think I find it understandable now, in the context of
eclass files.

-- 
konsolebox

Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[Mike Gilbert]

On Wed, Nov 30, 2016 at 7:11 AM, konsolebox  wrote:
>>> I also prefer some things this way:
>>>
>>> - Indent the contents of the first `if` block for consistency's sake,
>>> and less confusion.
>>
>> I disagree; indenting the entire eclass is silly and does not really
>> improve readability. Also, this is a very common pattern found in
>> other eclasses.
>
> I prefer following consistency before anything else.  And it's just
> uncommon and odd, but it's not silly.  Imagine if you use another `if`
> block on the second level where more functions are defined.  Would you
> also not indent that?

That first "if" is a bit of a special case; it's only there to prevent
the code from being executed more than once in global scope. This
provides a minor speed boost when the eclass gets sourced more than
once, and makes sure that any global variables are only set once.

I think of it as being similar to pre-processor statement you would
find in a C header file; one does not generally indent all the code
within a C header file, because it just introduces a big chunk of
whitespace that does not improve code readability.

Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[konsolebox]

On 11/30/16, Mike Gilbert  wrote:
> On Wed, Nov 30, 2016 at 3:38 AM, konsolebox  wrote:
>> - `[[ ${ROOT} == / ]] || return 0` seems to present a harmless false
>> condition, and it doesn't show an error message.  I would be helpful
>> to have a comment added above it to give details why.
>
> We only want to process tmpfiles for the currently running system.
>
> If ROOT is not /, it indicates we are installing the package for use
> on a different system or in a container. In either case, the tmpfiles
> would be processed upon boot when that system/container is started.
>
> I find this fairly obvious, but if William wants to document it that's
> fine.

I'm not familiar with that, and so would other scripters who would
look at the eclass.

>> I also prefer some things this way:
>>
>> - Indent the contents of the first `if` block for consistency's sake,
>> and less confusion.
>
> I disagree; indenting the entire eclass is silly and does not really
> improve readability. Also, this is a very common pattern found in
> other eclasses.

I prefer following consistency before anything else.  And it's just
uncommon and odd, but it's not silly.  Imagine if you use another `if`
block on the second level where more functions are defined.  Would you
also not indent that?

>> - Patterns in the `case` block doesn't have to be indented. This makes
>> the contents of the `case` block aligned with the contents of the
>> other blocks (`if`, `while`, etc.), and it makes the use of indents at
>> minimum when the block is used recursively.
>
> Sorry, I have no idea what you're trying to say here. Recursive blocks?

One expensive in the use of indents:

case $x in
a)
# first inner level
case $y in
1)
# second inner level
case ...
;;
2)
;;
esac
;;
b)
;;
esac

if [ "$x" = a ]; then
# first inner level
if [ "$y" = 1 ]; then
# second inner level
elif [ "$y" = 2 ]; then
:
else
:
fi
elif [ "$x" = b ]; then
:
else
:
fi

vs.

case $x in
a)
case $y in
1)
case ...
;;
2)
;;
esac
;;
b)
;;
esac

> This really feels like you're making a personal style suggestion here,
> and I personally see nothing wrong with it as-is.

Yes the second part is more personal.

-- 
konsolebox

Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[Mike Gilbert]

On Wed, Nov 30, 2016 at 3:38 AM, konsolebox  wrote:
> There are some things I noticed in the tmpfiles_process() function:
>
> - `type` currently also checks for functions, alias, and builtins,
> besides executable files.  If that's not intended, the `-P` option
> should be added.

I cannot conceive of any way a function or alias would be defined with
these names unless an ebuild author did it intentionally or a user did
it via /etc/portage/bashrc. In either case, I see no need to prevent
that.

> - `[[ ${ROOT} == / ]] || return 0` seems to present a harmless false
> condition, and it doesn't show an error message.  I would be helpful
> to have a comment added above it to give details why.

We only want to process tmpfiles for the currently running system.

If ROOT is not /, it indicates we are installing the package for use
on a different system or in a container. In either case, the tmpfiles
would be processed upon boot when that system/container is started.

I find this fairly obvious, but if William wants to document it that's fine.

> I also prefer some things this way:
>
> - Indent the contents of the first `if` block for consistency's sake,
> and less confusion.

I disagree; indenting the entire eclass is silly and does not really
improve readability. Also, this is a very common pattern found in
other eclasses.

> - Patterns in the `case` block doesn't have to be indented. This makes
> the contents of the `case` block aligned with the contents of the
> other blocks (`if`, `while`, etc.), and it makes the use of indents at
> minimum when the block is used recursively.

Sorry, I have no idea what you're trying to say here. Recursive blocks?

This really feels like you're making a personal style suggestion here,
and I personally see nothing wrong with it as-is.

Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[konsolebox]

There are some things I noticed in the tmpfiles_process() function:

- `type` currently also checks for functions, alias, and builtins,
besides executable files.  If that's not intended, the `-P` option
should be added.
- What happens if neither systemd-tmpfiles nor opentmpfiles is found?
- Shouldn't the function return a nonzero if an error occurs?  It
would help scripts abort.
- `[[ ${ROOT} == / ]] || return 0` seems to present a harmless false
condition, and it doesn't show an error message.  I would be helpful
to have a comment added above it to give details why.

I also prefer some things this way:

- Indent the contents of the first `if` block for consistency's sake,
and less confusion.
- Patterns in the `case` block doesn't have to be indented. This makes
the contents of the `case` block aligned with the contents of the
other blocks (`if`, `while`, etc.), and it makes the use of indents at
minimum when the block is used recursively.
- The subject word in the case block does not have to be quoted.
- Always keep blocks isolated from adjacent lines.

-- 
konsolebox


tmpfiles.eclass
Description: Binary data