1 |
On Mon, 12 Sep 2011 16:00:20 -0500 |
2 |
Donnie Berkholz <dberkholz@g.o> wrote: |
3 |
|
4 |
> > # @FUNCTION: remove_libtool_files |
5 |
> > -# @USAGE: [all|none] |
6 |
> > +# @USAGE: [all|only-not-required|none] |
7 |
> |
8 |
> Is there a way to document the arguments of eclass functions? You |
9 |
> added the name of the arg but didn't describe its purpose or why |
10 |
> anyone would want to use it. |
11 |
> |
12 |
> On a semantic note, that argument name (only-not-required) doesn't |
13 |
> make sense to me. I might do something more helpful like |
14 |
> pkgconfig-duplicates instead. |
15 |
|
16 |
I thinked about 'as-needed' or sth like that. Maybe the new argument |
17 |
should be (temporarily) not public instead? |
18 |
|
19 |
> > + if [[ "$1" == 'only-not-required' ]]; then |
20 |
> |
21 |
> This is way more quoting than you need within double brackets. |
22 |
|
23 |
It's nice visual quoting, just to match the others. |
24 |
|
25 |
> > local f |
26 |
> > for f in $(find "${D}" -type f -name '*.la'); do |
27 |
> > # Keep only .la files with shouldnotlink=yes - |
28 |
> > likely plugins local shouldnotlink=$(sed -ne |
29 |
> > '/^shouldnotlink=yes$/p' "${f}") if [[ "$1" == 'all' || -z |
30 |
> > ${shouldnotlink} ]]; then |
31 |
> > + if [[ "$1" == 'only-not-required' ]]; then |
32 |
> |
33 |
> Is there a case where one of those arguments might be $2 but you'd |
34 |
> still want to run this? |
35 |
|
36 |
Er? What are you referring to? |
37 |
|
38 |
> I feel like that shouldnotlink thing is really confusing the logic, |
39 |
> because there's multiple nested tests for different values of $1 in |
40 |
> here instead of just testing the args once at the top and setting |
41 |
> variables. |
42 |
|
43 |
As mentioned earlier, the code needs to be refactored. First things |
44 |
first, then we'll rewrite it to be nice and clean. I don't really want |
45 |
to waste time doing this if we would need to rewrite it for more logic |
46 |
in the future. |
47 |
|
48 |
> > + # remove .la files only when .pc |
49 |
> > files provide the libs |
50 |
> > + # already or they don't give any |
51 |
> > information |
52 |
> > + ! has $(basename "${f}") |
53 |
> > ${pc_libs} \ |
54 |
> > + && [[ -n "$(sed -n |
55 |
> > \ |
56 |
> |
57 |
> The comment says "or" but I see an "and" here. |
58 |
|
59 |
Because everything's negated here. Boolean magic :D. |
60 |
|
61 |
-- |
62 |
Best regards, |
63 |
Michał Górny |