Jason van Zyl wrote:
>
> "Geir Magnusson Jr." wrote:
>
> > [SNIP]
> > Can we just support both then? The 'same value key' problem is somewhat
> > artificial, because you and john wanted to do, for multiple paths,
> >
> > path = foo
> > path = bar
> > path = woogie
>
> Which is equivalent to
> path = foo,bar,woogie
>
> With the Configuration class, and we wanted it because we use
> that in turbine and the classes have been used for a long time and
> work.
Ah. I looked in turbine, and didn't see that. Did see a Properties
though.
> >
> >
> > rather than
> >
> > path = foo:bar:woogie
> >
> > or
> >
> > path.1 = foo
> > path.2 = bar
> > path.3 = woogie
> >
> > (The latter way is nicer - you don't have to worry that the separator
> > is a valid character in the data )
>
> This will not work. The name of the property has to be the same in
> order to dynamically set the resource path. In Turbine we might
> do:
Sorry. Of course I meant :
resource.loader.1.resource.path.1 = foo
resource.loader.1.resource.path.2 = bar
(just too lazy to type it)
> 1)
> Runtime.setSourceProperty(FILE_RESOURCE_PATH, path1);
> Runtime.setSourceProperty(FILE_RESOURCE_PATH, path2);
>
> I do not want to have to do
>
> 1)
> Runtime.setProperty("resource.loader.1.file.resource.path.1", path1);
> Runtime.setProperty("resource.loader.1.file.resource.path.2", path1);
>
> I don't have a safe constant to work with, and the file loader might not be
> the first loader. If the defaults for the file resource loader get overriden
> then this code will break. 1) is always guaranteed to work.
I still think that we should punt on these methods to Runtime since a
single call to init() can do all of this, and I think that hardwiring
FILE_RESOURCE_PATH as a constant isn't a good idea as it isn't safe
because it makes assumptions about the public.name of the loader...but
ignoring that, what do these really have to do with each other?
FILE_LOADER_RESOURCE_PATH isn't defined as
"resource.loader.1.file.resource.path". It is "file.resource.path",
which isn't a real property for configuring a loader. This is a
source-specific mechanism dependant upon the ResourceManager taking it
apart to deduce the public name of the loader. The other part
'resource.path' is passed into the Configuration object handling that
loader, and it does the right thing, tacking it on the end. I do like
this hack, and the Configuration class seems rather nice, BTW.
But, that said, making the props
resource.loader.1.resource.path.1 = foo
resource.loader.1.resource.path.2 = bar
would have no effect on that. It would require a change in loading the
Configuration to deal with the trailing integer, bit you already are
dealing with the complexity of the multiple resource loader definitions
and ordering.
What I am saying is that changing the multi-path property to be as I
suggest would have *no* effect on the way Turbine prefers to load
things, or those Runtime methods in general. They wouldn't have to
change at all. The only thing that would change is how the
Configuration / Configurations / ExtendedProperties deals with the new
property spec.
>
> >
> >
> > If we switched to the second way, then both the Configuration and the
> > Properties would work. I do agree that it's a shame there isn't
> > multi-valued property support in Properties.
>
> No, they wouldn't.
I think they could.
> >
> >
> > > > The multi-key problem only surfaced recently with the multi-node
> > > > template and jar paths, and that was solvable in the same way WM solved
> > > > it, by letting the property be free form, and letting the entity that
> > > > required multi-valued properties to parse in the way it needs.
> > >
> > > The Configuration class always dealt with multiple keys correctly, it's the
> > > Properties
> > > class that doesn't. It should have been something like init(Configuration) from
> > > the start. But it didn't even cross my mind at the time, I just think this is
> > > the correct way to go and is used the same way a Properties class is so it's
> > > not like there's a big learning curve or anything :-)
> >
> > It's not the learning curve : I just don't want to force people to use
> > it. I am just arguing to keep it, and add an init( Configuration ), but
> > keeping init( Properties ) would be easy for those using the Properties
> > in their own apps.
>
> That's why I want to do the change now before we launch. Get
> rid of it now while we can!
I just don't think we need to force everyone to use an entirely new
class (for everyone except Turbine - and currently, Turbine doesn't use
it to init Velocity, and the Velocity version isn't the same as the
Turbine version) to handle application-level properties specification
methods, since I believe (and am happy to prove) that we could easily
acommodate both.
I just don't see the downside - I think we can get everything we want at
once - arrange things so that we support what I suppose lots of Java
programmers (including the person who wrote TurbineVelocityService) use
for application configuration... java.util.Properties.
> > > >
> > > > Everything that we want to do could be achieved with the
> > > > java.util.Properties.
> > >
> > > I don't think so, the Properties class doesn't behave the way the
> > > Configuration class does and it's the Configuration class that has
> > > always been the core configuration class. To make a Properties
> > > class work with the Configuration class you would have to add
> > > a hack in the runtime to parse CSV lines when the Configuration
> > > class does this already.
> >
> > Or if we made a simple change to what a multi-path spec is, to
> >
> > path.1 = foo
> > path.2 = bar
> >
> > wouldn't the issue just go away?
>
> No, as explained above.
I wasn't convinced - see above.
> >
> >
> > > > If Configuration makes life easier internally to
> > > > justify the added code, then I am all for it, but I still think that we
> > > > should support the standard Properties object as a full functional
> > > > method of init.
> > >
> > > Just for the record, I don't consider the using a Properties object to
> > > init the runtime the standard way. The app that has been using Velocity
> > > the longest, turbine, has never used this method.
> >
> > And looking at TurbineVelocityService, it uses a Properties internally
> > (so some apps *do* use the Properties class for configuration
> > management), and knows where its properties file is. I think the
> > following code would make Turbine compatible with using j.u.Properties
> > to initialize :
>
> It doesn't work the way you think it does.
Ok. I figured that I messed it up somewhere - The current Turbine code
is :
/*
* Get the properties for this Service
*/
Properties props = getProperties();
String vProperties = TurbineServlet.getRealPath(
props.getProperty("properties", null));
String vLog = TurbineServlet.getRealPath(
props.getProperty("log", null));
String vTemplates = TurbineServlet.getRealPath(
props.getProperty("templates", null));
[SNIPPING UNRELATED CODE FOR BREVITY]
try
{
Runtime.setProperties(vProperties);
Runtime.setSourceProperty(Runtime.FILE_RESOURCE_LOADER_PATH,
vTemplates);
Runtime.setProperty(Runtime.RUNTIME_LOG, vLog);
Runtime.init();
encoding = Runtime.getString(Runtime.TEMPLATE_ENCODING);
Log.info("VelocityService: encoding=" + encoding);
}
catch(Exception e)
...
And I suggested, for placement in the try block :
> >
> > Properties p = new Properties();
> > p.load( vProperties );
> > p.setProperty(Runtime.FILE_RESOURCE_LOADER_PATH, vTemplates);
> > p.setProperty(Runtime.RUNTIME_LOG, vLog);
> > Runtime.init();
>
> There is no velocity properties file.
Well, you are setting a set of properties in the first line in the try
block - but I know that - you depend upon the velocity defaults.
But yes, I see that it wouldn't work as suggested, because of the
dependence upon the default properties, so the first setProperty()
wouldn't do the right thing, as FILE_RESOURCE_LOADER_PATH isn't a real
property.
One thing you could do is change the ResourceManager to handle the magic
cookies like FILE_RESOURCE_LOADER_PATH and deal with them as it does
now, but there is not point getting into that here.
You could keep the Turbine code the same, change the resource path
property to include the trailing integer, and people could still use
Properties.
> >
> >
> > It's one line of code more than the current implementation. It's
> > structurally the same : the current implementation just uses Runtime as
> > a Properties, and the above uses a Properties as a Properties.
>
> So why create another object if you don't have to?
If we only accepted a Configuration object, wouldn't everyone have to
create a new Configuration object and load it from their Properties?
And we are talking init() time anyway...
> >
> >
> > Of course, there may be something subtle about Turbine where that
> > wouldn't work. I am not sure.
> >
> > If we got rid of those methods from Runtime, Runtime would get smaller,
> > and clean up the initialization semantics w/o undue burden on Turbine.
> > Further, in general one wouldn't be tempted to call
> > Runtime.setProperty() after init(), since those methods wouldn't exist.
>
> But we very well may want to in the future.
I agree.
> >
> >
> > Currently, we don't support post-init() property additions anyway, so
> > nothing would be lost.
>
> But we would be losing the potential ability to configure the system
> while it's running. Say add a new template path, drop in some new
> templates and away you go. As part of a content management system
> that can't be shut down for re-configuration. Lots of possibilities
> there. If anything I say get rid of the init(Props) if we're really
> looking to clean things up.
But they wouldn't work anyway now : the loaders are all set up and
initted...
I agree it's a good thing and we want to do it - but we would have to
fix a few things for it to work.
> >
> >
> > The rule would be 1) optionally setup the properties (in a Properties
> > or Configuration) and then 2) call init() [with no arg for defaults,
> > with a filename for vel to read, with a Properties if you have one, or a
> > Configuration if you use that.]
> >
> > And I don't consider either the 'standard way' - I just see the
> > java.util.Properties as an easy way using a standard class that people
> > could already use in their own applications (like Turbine seems to) - so
> > they could just glom together the velocity properties with their
> > properties for easy configuration, and just use that.
> >
> > I think giving people the choice of
> >
> > init() : use defaults
> > init( filename ) : use filename
> > init( Properties ) :
> > init( Configuration );
>
> >
> > would make it more convenient.
>
> If the init(Props) is removed before we release then people won't
> know what they're missing. I would say deprecate the use of
> init(Props) to give people fair warning.
All I am saying is that we don't have to - we could work it out, or I
believe we could and am willing to help, try to get this to work where
we *can* support init(Props).
Is there any other downside other than the current bit regarding
multi-path, which I believe we can fix?
geir
--
Geir Magnusson Jr. [EMAIL PROTECTED]
Developing for the web? See http://jakarta.apache.org/velocity/
