Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.x
    • Fix Version/s: 2.x
    • Component/s: Engine
    • Labels:
      None

      Description

      Currently the whole configuration of Velocity Engine is managed by ExtendedProperties, a class from Commons Collections.
      An interface that abstracts from this implementation, and an implementation that uses normal Properties should be used.

        Issue Links

          Activity

          Hide
          Claude Brisson added a comment -

          Any progress on this one, Adrian?

          Show
          Claude Brisson added a comment - Any progress on this one, Adrian?
          Hide
          Adrian Tarau added a comment -

          It seems like ResourceManagerImpl uses subset so I will keep this feature.

          ExtendedProperties loaderConfiguration =
          rsvc.getConfiguration().subset(loaderID.toString());

          Show
          Adrian Tarau added a comment - It seems like ResourceManagerImpl uses subset so I will keep this feature. ExtendedProperties loaderConfiguration = rsvc.getConfiguration().subset(loaderID.toString());
          Hide
          Nathan Bubna added a comment -

          Yeah, no null booleans in config, please. And the only type conversion in Velocity internals is toString(), i believe.

          Show
          Nathan Bubna added a comment - Yeah, no null booleans in config, please. And the only type conversion in Velocity internals is toString(), i believe.
          Hide
          Adrian Tarau added a comment -

          Also does it make sense for Velocity "Boolean getBoolean(String key, Boolean defaultValue)" to allow a null value for a configuration key?

          I would say "boolean getBoolean(String key, boolean defaultValue)" should be enough...a "primitive" type should not be allowed to have a null value, default/safe value should be provided all the time.

          Show
          Adrian Tarau added a comment - Also does it make sense for Velocity "Boolean getBoolean(String key, Boolean defaultValue)" to allow a null value for a configuration key? I would say "boolean getBoolean(String key, boolean defaultValue)" should be enough...a "primitive" type should not be allowed to have a null value, default/safe value should be provided all the time.
          Hide
          Adrian Tarau added a comment -

          Only the configuration...for now. I was trying to find where Velocity do some type conversion and I didn't saw anything to suggest that(didn't looked to much either). I would expect when invoking $obj.setThing($thing) with setThing(int) and type($thing) = String to have an automatic type conversion(if possible otherwise exception). Is either I was type safe all the time or either there is a type conversion somewhere

          Show
          Adrian Tarau added a comment - Only the configuration...for now. I was trying to find where Velocity do some type conversion and I didn't saw anything to suggest that(didn't looked to much either). I would expect when invoking $obj.setThing($thing) with setThing(int) and type($thing) = String to have an automatic type conversion(if possible otherwise exception). Is either I was type safe all the time or either there is a type conversion somewhere
          Hide
          Nathan Bubna added a comment -

          What's the benefit of having all those converters?

          Show
          Nathan Bubna added a comment - What's the benefit of having all those converters?
          Hide
          Adrian Tarau added a comment -

          I'm thinking to add a dependency to BeanUtils's converters package(this dependency will not be propagated within Maven, it will be used just to build Velocity and at runtime the converters packages will be part of Velocity under org.apache.velocity.converters using shading) for type conversion in Configuration. This will add 47kb in compressed byte code but I think it's worth it...if not approved I will go with a simpler type conversion(only from String to type XXX).

          http://commons.apache.org/beanutils/v1.8.3/apidocs/index.html?org/apache/commons/beanutils/converters/package-summary.html

          Show
          Adrian Tarau added a comment - I'm thinking to add a dependency to BeanUtils's converters package(this dependency will not be propagated within Maven, it will be used just to build Velocity and at runtime the converters packages will be part of Velocity under org.apache.velocity.converters using shading) for type conversion in Configuration. This will add 47kb in compressed byte code but I think it's worth it...if not approved I will go with a simpler type conversion(only from String to type XXX). http://commons.apache.org/beanutils/v1.8.3/apidocs/index.html?org/apache/commons/beanutils/converters/package-summary.html
          Hide
          Adrian Tarau added a comment -

          I will drop them

          Show
          Adrian Tarau added a comment - I will drop them
          Hide
          Nathan Bubna added a comment -

          No need for them with regard to configuration.

          Show
          Nathan Bubna added a comment - No need for them with regard to configuration.
          Hide
          Adrian Tarau added a comment -

          Any need for BigDecimal and BigInteger in Velocity?

          Show
          Adrian Tarau added a comment - Any need for BigDecimal and BigInteger in Velocity?
          Hide
          Adrian Tarau added a comment -

          During the implementation I started with a few suppositions:

          • used Apache Commons Configuration(http://commons.apache.org/configuration/apidocs/index.html), it seems like a good model to me(I use it in my projects and it proved to be well defined)
          • create the smallest API possible without dropping useful functionality
          • removed the SubConfiguration it doesn't seems to provide much for Velocity
          • removed getTypeXX(key), Velocity should start with default values if no configuration is provided, no failures allowed if a configuration is missing(however a ConversionException will be thrown if a conversion to a specific type cannot be performed)
          • kept the concept of a configuration listener just in case in the future Velocity will reload itself(parts) at runtime if configuration changes. For performance reasons you don't want to get a configuration value at runtime, you load it during initialisation in a local(instance) variable and register a listener to the configuration if you want to be notified when the configuration changes(to reinitialise your internals)
          • kept the concept of keeping the value of a key as an Object, allowing to pass Lists. Also getStringArray/setStringArray looks for a separator(default ",") to get/set lists.
          • no support for interpolation, implementations like Apache Commons Configuration will provide such a functionality
          • provide a ConfigurationFactory as an entry point to get a configuration(based on a Map, a File or a String) and all implementations are private. Developers can provide obviously their own Configuration implementation and the only restriction is to thread safe(see configuration listener)

          Any comments are welcomed.

          Show
          Adrian Tarau added a comment - During the implementation I started with a few suppositions: used Apache Commons Configuration( http://commons.apache.org/configuration/apidocs/index.html ), it seems like a good model to me(I use it in my projects and it proved to be well defined) create the smallest API possible without dropping useful functionality removed the SubConfiguration it doesn't seems to provide much for Velocity removed getTypeXX(key), Velocity should start with default values if no configuration is provided, no failures allowed if a configuration is missing(however a ConversionException will be thrown if a conversion to a specific type cannot be performed) kept the concept of a configuration listener just in case in the future Velocity will reload itself(parts) at runtime if configuration changes. For performance reasons you don't want to get a configuration value at runtime, you load it during initialisation in a local(instance) variable and register a listener to the configuration if you want to be notified when the configuration changes(to reinitialise your internals) kept the concept of keeping the value of a key as an Object, allowing to pass Lists. Also getStringArray/setStringArray looks for a separator(default ",") to get/set lists. no support for interpolation, implementations like Apache Commons Configuration will provide such a functionality provide a ConfigurationFactory as an entry point to get a configuration(based on a Map, a File or a String) and all implementations are private. Developers can provide obviously their own Configuration implementation and the only restriction is to thread safe(see configuration listener) Any comments are welcomed.
          Hide
          Adrian Tarau added a comment -

          Usually it is worth it...Code gets "dirty" and a clean-up is at least recommended if not required .

          Show
          Adrian Tarau added a comment - Usually it is worth it...Code gets "dirty" and a clean-up is at least recommended if not required .
          Hide
          Nathan Bubna added a comment -

          Yeah, but is it worth it at this point?

          Show
          Nathan Bubna added a comment - Yeah, but is it worth it at this point?
          Hide
          Adrian Tarau added a comment -

          Well, 2.0 might be the version where a little bit of refactoring would not hurt

          Show
          Adrian Tarau added a comment - Well, 2.0 might be the version where a little bit of refactoring would not hurt
          Hide
          Nathan Bubna added a comment -

          I prefer org.apache.velocity.configuration.

          I have to constantly fight the urge to abolish both the o.a.v.app and o.a.v.runtime packages. Too many of the packages under o.a.v. are unnecessary, IMO.

          Show
          Nathan Bubna added a comment - I prefer org.apache.velocity.configuration. I have to constantly fight the urge to abolish both the o.a.v.app and o.a.v.runtime packages. Too many of the packages under o.a.v. are unnecessary, IMO.
          Hide
          Adrian Tarau added a comment -

          Any suggestions for the configuration package?
          1. org.apache.velocity.configuration
          2. org.apache.velocity.app.configuration
          3. org.apache.velocity.runtime.configuration

          I would say org.apache.velocity.runtime.configuration ...

          Show
          Adrian Tarau added a comment - Any suggestions for the configuration package? 1. org.apache.velocity.configuration 2. org.apache.velocity.app.configuration 3. org.apache.velocity.runtime.configuration I would say org.apache.velocity.runtime.configuration ...
          Hide
          Antonio Petrelli added a comment -

          Since I didn't start yet, it's all yours :-D

          Thanks

          Show
          Antonio Petrelli added a comment - Since I didn't start yet, it's all yours :-D Thanks
          Hide
          Adrian Tarau added a comment -

          Can I take this one?

          Show
          Adrian Tarau added a comment - Can I take this one?

            People

            • Assignee:
              Adrian Tarau
              Reporter:
              Antonio Petrelli
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development