"Geir Magnusson Jr." wrote:
> 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.
>
> > >
> > >
> > > 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?
We agreed that the file resource loader was a standard loader did we not?
And as a standard loader the setting of its configuration can be set
as it is done above. I think the class resource loader should be a standard
loader too. With these two, I think 99% of most cases are handled.
>
>
> 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.
I don't consider it a hack, I think it provides the easiest way to
embed velocity. No external velocity props is required, you
have access to the standard loaders and you can set the
config for the loader very easily using the constant.
>
>
> 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.
Yes it would have an effect. Turbine doesn't have a velocity properties
file and depends on the the defaults that velocity provides. Now for
Turbine I'm on top of the changes but with your method I would have
to the
Runtime.setProperties("resource.loader.1.resource.path.1", path1)
But this requires people to dig through the source and find out
what the defaults are to get the property right. Why should I
have to know what that string is? I just want to set a resource
path.
And if we decided to change the defaults, people who didn't stay
on top of changes would find their apps broken.
So let me get this clear, apps that have no velocity properties
are supposed use
Runtime.setProperties("resource.loader.1.resource.path.1", path1)
to set a resource path?
>
>
> 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 I'm correct in how I think you want to specify a resource path
then it certainly has an effect. But clarify this if I misunderstand.
>
>
> >
> > >
> > >
> > > 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.
We're not forcing people to do anything, we haven't even released?
Sure you could accomdate it with free text definition of properties
but I don't think that's a good thing. I think the constant is a much
safer mechanism. Users of velocity will use the file loader that we
provide and I think the way turbine uses velocity is the most
convenient.
How about we poll the users? I think the way turbine does it is easiest,
you use properties. How about we ask the users?
>
>
> 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.
If I have to use free text definition of properties then I definitely
have a problem with it.
>
>
> > > > >
> > > > > 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.
The velocity properties file is empty, that line will go away.
Take a look at an example turbine app and you will see that.
What you are looking at will become
Runtime.setResourceProperty(Runtime.FILE_RESOURCE_LOADER_PATH, path)
Runtime.setProperty(Runtime.RUNTIME_LOG, log)
Runtime.init()
I think this is the simplest, most reliable way to embed velocity. I don't
need to create an object to initialize. And for each other path I just
add another line like the first. How much easier could it get?
>
>
> 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.
Of course it's a real property? Shall we make people look up what the
property is for the log file? In order to set it's value?
> You could keep the Turbine code the same, change the resource path
> property to include the trailing integer, and people could still use
> Properties.
So something like this:
Runtime.setResourceProperty(SOME_CONSTANT + ".1", path1)
Here we are protected against changes in the defaults, but looks
terrible compared to the method that will be used.
or are you suggesting:
Runtime.setResourceProperty("resource.loader.1.resource.path.1", path1)
which is just plain dangerous because the defaults may change. using
free text to set a runtime property is not a good idea.
>
>
>
> > >
> > >
> > > 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...
They would use it exactly as a properties file. I don't see
the problem.
>
>
> > >
> > >
> > > 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.
>
> > >
> > >
> > > 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?
I will not accept using free text for setting runtime properties, that
is my objection. And I think we feel differently about the status
of loaders. I feel that the file loader and the class loader deserve
to be first class, they are core to velocity and providing constants
for their use provides the best way to embed velocity.
I don't need a properties file, but in order to accommodate them
I would have to use a method initialization that I think is dangerous
i.e. using free text to set the resource paths. That's the only way
I see your way working. If you use a constant then your back
doing what is already done. I think the constant is good and
I think the idea of standard class loaders is good.
I don't think making people use a Configuration class
is really a big deal. If we can't get this sorted out before
tomorrow night then we can just remove from the todo
and concentrate on the other items for release.
>
--
jvz.
Jason van Zyl
[EMAIL PROTECTED]
http://jakarta.apache.org/velocity
http://jakarta.apache.org/turbine
