Hi Olivier,
Thanks for explaining!
My opinion is that if j.l.String is used properly then it is sufficient
for this use case. One benefit in your solution is the parsing of the
JVM/web init-param property. Now it warns when unknown value is
provided.
There are few comments about the patch though:
- it seems you don't use the Eclipse's preferences which come with
Wicket's code. I guess you use another IDE. All imports are folded
(+ import org.apache.wicket.*) and many indentations are broken
- there is a dead local variable in the code related to parsing of
JVM/init-param property
Keep the good work!
On Wed, 2009-11-11 at 17:26 +0100, Olivier Croisier wrote:
> The benefit is that the code doesn't rely on string literal
> comparison, which is inherently neither secure nor
> refactoring-friendly.
> Ex: Before my patch, there was at least one == string comparison,
> which result may vary depending on whether the strings are intern()-ed
> or not.
> Enums prevent that sort of errors, and ensure that constants are
> defined in only one place (the enum class) instead of maybe being
> scattered across many classes (you can define string literals
> anywhere).
>
> BTW, I'm not sure the configuration type modularization, as proposed
> by WICKET-1847, is a good idea.
> Let me explain my point of view :
> IMO, the runtime level belongs to wicket internals and shall not be
> used by users to tweak their applications. And for the Wicket engine,
> only two modes are relevant : debug ("development") and "not
> debug" (ie, deployment). Also, these modes are referred to in many
> places throughout the code ; allowing user-defined runtime types would
> require to give them access to lots of Wicket's internals. There are
> already too many special corner cases in the main and sub-projects
> with the existing two modes.
>
> So I think the user customisation, if really needed, would be better
> implemented by another system ("profiles" ?) that would deal only with
> the startup configuration (ie, tune the various *Settings), and would
> be applied after the existing development/deployment default
> configuration. Such profiles could be detected using the Service
> Provider API for example, and selected by an init-parameter.
>
> What do you think ?
>
> Olivier
>
>
>
>
>
>
> On Wed, Nov 11, 2009 at 4:08 PM, Martin Grigorov <mcgreg...@e-card.bg>
> wrote:
> On Wed, 2009-11-11 at 15:59 +0100, Olivier Croisier wrote:
> > Hi,
> >
> > WICKET-1847 is more than a year old and has had no activity
> for more than a
> > year, whereas the WICKET-1945 was more revent and seemed
> interesting to me,
> > so I corrected it.
> > But in my opinion, even if the former is the target
> architecture, nothing
> > prevents from integrating my patch now (in the 1.4.x branch
> for example) and
> > benefit from the type safety in the meantime.
>
> What is the actual benefit of this type safety?
>
> If it is prefered to be enum I would prefer to use
> org.apache.wicket.util.lang.EnumeratedType which is
> extendable.
>
> >
> > Olivier
> >
> >
> > On Wed, Nov 11, 2009 at 3:42 PM, Jonas <barney...@gmail.com>
> wrote:
> >
> > > I thought the plan for 1.5 was going into the opposite
> direction:
> > > https://issues.apache.org/jira/browse/WICKET-1847
> > >
> > > On Wed, Nov 11, 2009 at 3:34 PM, Olivier Croisier
> > > <olivier.crois...@gmail.com> wrote:
> > > > Hi,
> > > >
> > > > I just submitted a patch that converts the runtime
> configuration types
> > > > ("development", "deployment") into a type-safe Enum.
> > > > See https://issues.apache.org/jira/browse/WICKET-1945
> > > >
> > > > Hope that helps,
> > > > Olivier
> > > >
> > >
>
>
>
>
