data_smart: Drop default expand=False to getVar [API change]

At some point in the future, getVar should expand by default. To get there from the current position, we need a period of time where the expand parameter is mandatory.

This patch starts that process. Clear errors will result from any code which doesn't provide this. Layers can be fixed with an expression like:

sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i grep -ril getVar *

Signed-off-by Richard Purdie <[email protected]>

Quoted 10 lines · Richard Purdie

At some point in the future, getVar should expand by default. To get
there from the current position, we need a period of time where the expand parameter is mandatory.

This patch starts that process. Clear errors will result from any code
which doesn't provide this. Layers can be fixed with an expression like:

Quoted 4 lines

sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i grep -ril getVar *

Signed-off-by Richard Purdie <[email protected]>

I am in favor of making it mandatory but I fail to see what we gain in making it expand by default in future.

Quoted 15 lines · Otavio Salvador

Quoted 13 lines · Richard Purdie

At some point in the future, getVar should expand by default. To get
there from the current position, we need a period of time where the expand parameter is mandatory.

This patch starts that process. Clear errors will result from any code
which doesn't provide this. Layers can be fixed with an expression like:

sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i grep -ril getVar *

Quoted 4 lines

Quoted 1 line

Signed-off-by Richard Purdie <[email protected]>

I am in favor of making it mandatory but I fail to see what we gain in making it expand by default in future.

Less ugly syntax, in 99.9% of usages, you would want expand=True and getVar("X") is neater than getVar("X", True).

Quoted 8 lines · Richard Purdie

Quoted 3 lines · Otavio Salvador

I am in favor of making it mandatory but I fail to see what we gain
in making it expand by default in future.

Less ugly syntax, in 99.9% of usages, you would want expand=True and
getVar("X") is neater than getVar("X", True).

Consistent indentation is also neater than mixing tabs and spaces, but
you were refusing to take such cosmetic change and now you're proposing
neater getVar syntax which is (in some cases silently) breaking existing
metadata and make backporting changes more difficult?

I'm not opposing the change, just don't understand why the same arguments work here and not for indentation.

Quoted 18 lines · Martin Jansa

Quoted 8 lines · Richard Purdie

Quoted 3 lines · Otavio Salvador

I am in favor of making it mandatory but I fail to see what we gain
in making it expand by default in future.

Less ugly syntax, in 99.9% of usages, you would want expand=True and
getVar("X") is neater than getVar("X", True).

Consistent indentation is also neater than mixing tabs and spaces, but
you were refusing to take such cosmetic change and now you're proposing
neater getVar syntax which is (in some cases silently) breaking existing
metadata and make backporting changes more difficult?

Quoted 2 lines

I'm not opposing the change, just don't understand why the same arguments work here and not for indentation.

If we go through the full transition, its possible something might silently break, if they skip the release where we require a value. We haven't reached that point yet and I'm honestly not sure how long we should wait, I'm open to suggestions. The aim is to minimise any silent failures though.

I'd also add that in many cases, it likely won't break at all, it might potentially fix something. If there are concerns about this, there are extra things we could consider such as requiring layers to mark they've been validated for the change. That adds extra complexity, I'm less sure about whether we need to do that.

As for your other point, whitespace in shell functions doesn't seem to cause real world usability issues in that most people can read a tab indented or space indented function equally well. OE-Core is pretty consistent about what it uses and always has been, you personally just don't like the particular choice made and meta-oe deciding to do something else, contra to the TSC doesn't exactly help consistency. You could argue meta-oe is more of the problem here.

The actual getVar syntax on the other handy is more than cosmetic, people don't understand what this "True" they keep adding means, or why they need it. You tend to know when you don't want expansion on the other hand. So a cleaner syntax which does what the user most likely needs is more than cosmetic.

Quoted 7 lines

The actual getVar syntax on the other handy is more than cosmetic,
people don't understand what this "True" they keep adding means, or why
they need it. You tend to know when you don't want expansion on the other hand. So a cleaner syntax which does what the user most likely
needs is more than cosmetic.

I'd agree with that, passing booleans as arguments sucks all around, since it loses context. *If* someone is going to do it, it's best to pass it with the argument name in keyword form in the caller, and obviously we don't want to do that here.

That said, I'm rather inclined toward thinking about alternative DataSmart APIs in general, in the long term. For example, it's a MutableMapping, so it acts dict-like, so we should be able to use .get() and get an expanded value, and then the case would line up with PEP8 (CamelCase for functions is frowned upon). I.e. we could have get() and get_unexpanded().

Quoted 12 lines · Christopher Larson

Quoted 7 lines

The actual getVar syntax on the other handy is more than cosmetic,
people don't understand what this "True" they keep adding means, or why
they need it. You tend to know when you don't want expansion on the other hand. So a cleaner syntax which does what the user most likely
needs is more than cosmetic.

I'd agree with that, passing booleans as arguments sucks all around, since it loses context. *If* someone is going to do it, it's best to pass it with the argument name in keyword form in the caller, and obviously we don't want to do that here.

Quoted 6 lines

That said, I'm rather inclined toward thinking about alternative DataSmart APIs in general, in the long term. For example, it's a MutableMapping, so it acts dict-like, so we should be able to use .get() and get an expanded value, and then the case would line up with PEP8 (CamelCase for functions is frowned upon). I.e. we could have get() and get_unexpanded().

I'm just thinking out loud here, I think the above is worth seriously considering. One big advantage "getVar" has is that its pretty unique and the python parsing code therefore doesn't get a lot of false positives with function names. "get" on the other hand, I can see being a nightmare.

Quoted 16 lines · Richard Purdie

Quoted 15 lines · Otavio Salvador

Quoted 13 lines · Richard Purdie

At some point in the future, getVar should expand by default. To get
there from the current position, we need a period of time where the expand parameter is mandatory.

This patch starts that process. Clear errors will result from any code
which doesn't provide this. Layers can be fixed with an expression like:

sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i grep -ril getVar *

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Richard Purdie <[email protected]>

I am in favor of making it mandatory but I fail to see what we gain in making it expand by default in future.

Less ugly syntax, in 99.9% of usages, you would want expand=True and getVar("X") is neater than getVar("X", True).

Consistent indentation is also neater than mixing tabs and spaces, but you were refusing to take such cosmetic change and now you're proposing neater getVar syntax which is (in some cases silently) breaking existing metadata and make backporting changes more difficult?

I'm not opposing the change, just don't understand why the same arguments work here and not for indentation.

At some point in the future, getVar should expand by default. To get there from the current position, we need a period of time where the expand parameter is mandatory.

This patch starts that process. Clear errors will result from any code which doesn't provide this. Layers can be fixed with an expression like:

sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i grep -ril getVar *

Signed-off-by Richard Purdie <[email protected]>

bitbake/lib/bb/data_smart.py +0 -0

index 8c3e9ef..79bec1b 100644

Quoted 31 lines · On Tue, 02 Feb 2016 23:55:40 Richard Purdie wrote:

At some point in the future, getVar should expand by default. To get
there from the current position, we need a period of time where the expand parameter is mandatory.

This patch starts that process. Clear errors will result from any code
which doesn't provide this. Layers can be fixed with an expression like:

sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i `grep -ril getVar
*`

Signed-off-by Richard Purdie <[email protected]>

bitbake/lib/bb/data_smart.py +0 -1

index 8c3e9ef..79bec1b 100644
@@ -566,7 +566,7 @@ class DataSmart(MutableMapping):
                 if len(shortvar) == 0:
                     override = None
 
-    def getVar(self, var, expand=False, noweakdefault=False,
 parsing=False): +    def getVar(self, var, expand,
 noweakdefault=False,
 parsing=False): return self.getVarFlag(var, "_content", expand,

noweakdefault, parsing)

Quoted 1 line

def renameVar(self, key, newkey, **loginfo):

This change is reasonable and we've been discussing it on and off for years
now, so I'm generally in favour.

However, what I would really like to see us do is write a script that people
can use to upgrade their layers to accommodate this and any other breaking
changes so they don't have to go and figure out what to do themselves; the more
of these kinds of changes we make the more useful this script will be. I'll
file a bug.