[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [buckminster-dev] Property Expansion for Environment Variables
|
Hi Thomas,
please see comments below
Am 06.11.2010 08:20, schrieb Thomas Hallgren:
Hi Johannes,
There are two ways of resolving this. Either you add all properties to
the property sets that you want to use like you suggest, or you let the
ExpandingProperties recognize the ${env_var: prefix}. The advantage with
the former is that it is simple and that the environment is included
when iterating over the keys in the map. The advantage of the latter is
that it will keep the map small (there's some copying going on) and is
in line with current Eclipse functionality. My vote is on the latter
because the use of env_var is uncommon (you're the first one asking for
it the four years Buckminster has been active ;-) ) and we might have an
urge to enable more prefixes.
Perhaps we should even consider having the ExpandingProperties consult
the Eclipse property expander when we encounter a prefix? That way all
the eclipse specific properties would work. What do you think?
- thomas
What you say makes a lot of sense to me. Also consulting the Eclipse
property expander makes it very easy for users to contribute completely
new kinds of (dynamic) properties by creating an extension for
org.eclipse.core.variables.(dynamic/static)Variables and therefore
participating with very little effort in the buckminster property
expansion process.
The env_var prefix is contributed by org.eclipse.debug.core which is
part of a regular buckminster installation anyway, so no additional
bundles would be required for the desired functionality.
So far so good, but there's a few things I'm a little uncertain about:
1. API consistency
ExpandingProperties is often regarded as a simple Map from what I can
see in the source. Consulting the StringVariableManager during the
expansion but ignoring it during the other API methods might behave a
little inconsistent for API consumers. E.g.
"keySet() doesn't return my env_var:FOOBAR but 'expand' replaces the
value just fine"
Including the new variable scope in all methods might on the other hand
negate the advantage of keeping the map small.
2. The implementation
Using the StringVariableManager seems straight forward enough:
VariablesPlugin.getDefault().getStringVariableManager().performStringSubstitution(expression,
false);
But ExpandingProperties is one of the core classes of Buckminster. It's
widely used and it seems a rather sophisticated implementation to me.
However, there don't seem to be any real unit tests in place for this
and I'm somewhat concerned of what I could break if include the
StringVariableManager there. Especially if you prefer to not only
include it in the property expansion, but keep the API consistent with
the new property scopes (because that'd mean to adjust quite a few of
the methods at once).
To wrap it up:
If you think env_vars and custom vars are corner cases and the Eclipse
mechanism should only be consulted during the actual expansion but
ignored in all other methodes, then I'd assume my changes should be made
around here in ExpandingProperties#checkedExpand :
Object propVal = (props instanceof ExpandingProperties<?>) ?
((ExpandingProperties<?>) props).getExpandedProperty(propKey,
recursionGuard + 1) : props.get(propKey);
But if you'd prefer that the other methods also include the additional
variable scopes for sake of consistency, then I'm afraid I'll need some
advice on how to apply the changes without breaking any of the classes
that depend on ExpandingProperties :-/
Best regards,
Johannes
