By reverting the constructors from 6.x we will reintroduce https://issues.apache.org/jira/browse/WICKET-4972
Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov On Tue, May 26, 2015 at 2:55 PM, Martijn Dashorst < [email protected]> wrote: > While trying to fix the resulting compile errors of this change, I > found it hard to decipher what each parameter meant (which is why we > opted going for the fluent interface in the first place) > > So take this example for instance: > > addColumn(new CustomPropertyColumn<Signaal>( > new StringResourceModel( > "studentnumber", > null, > "studentnumber", > FooApp.get().getStudentTermUpperCase() + "number" > ), > new StringResourceModel( > "studentnumber", > null, > "studentnumber", > FooApp.get().getStudentTermUpperCase() + "nummer" > ), "event.student.studentnumber") > .setDefaultVisible(false)); > > addColumn(new CustomPropertyColumn<Signaal>( > new StringResourceModel( > "student", > null, > "student", > FooApp.get().getStudentTermUpperCase() > ), new StringResourceModel( > "student", > null, > "student", > FooApp.get().getStudentTermUpperCase() > ), > "event.student.person.fullName") > .setDefaultVisible(false)); > > If you need to fix the compile error, there is no guidance left for > figuring out which parameters are passed in (which was already a pain, > but you could conceivably figure it out by navigating to the relevant > constructor that Eclipse or IntelliJ was using. By breaking hard, the > transition is (perhaps needlessly) hard. > > We could reïnstate the old constructors from 6.19.0 and mark them as > deprecated with the hint to refactor them into the fluent interface. > This would make transitioning (much [1]) easier. > > What do you guys think? > > Martijn > > [1] depending on the amount of internationalisation and usage of > StringResourceModel, and whether you have crafted a wrapper around the > beast. > > > > > > > > On Fri, May 15, 2015 at 2:10 PM, Martijn Dashorst > <[email protected]> wrote: > > I'm +1 for Guillaume's proposal, even if it breaks API late in the > > game. I'd rather have that than having to go through all > > StringResourceModel's constructors trying to figure out if they break > > silently. > > > > Martijn > > > > > > On Thu, May 14, 2015 at 8:25 PM, Guillaume Smet > > <[email protected]> wrote: > >> Hi, > >> > >> Sven's proposal was to create a new PropertyResourceModel which is IMHO > orthogonal to whether we decide to fix the issue we have with the new > StringResourceModel signature. > >> > >> Sven's proposal was to remove the MessageFormat support altogether and > I think it's a quite disruptive change. > >> > >> I think moving StringResourceModel to a more fluid interface would be > useful anyway to avoid having applications silently failing after the > migration to 7. > >> > >> I can work on a pull request if we agree on this. > >> > >> -- > >> Guillaume > >> > >>> Le 14 mai 2015 à 08:55, Martin Grigorov <[email protected]> a > écrit : > >>> > >>> Hi, > >>> > >>> I am not sure why Sven didn't push his proposal (StringResourceModel): > >>> http://markmail.org/message/35vkmmao5dxrmno2 > >>> In this mail thread there was a discussion about such change (fluent > API). > >>> > >>> Martin Grigorov > >>> Wicket Training and Consulting > >>> https://twitter.com/mtgrigorov > >>> > >>> On Thu, May 14, 2015 at 12:42 AM, Martijn Dashorst < > >>> [email protected]> wrote: > >>> > >>>> Sounds like a good idea. We have a special I18N class in our app > >>>> specifically as a wrapper around StringResourceModel. > >>>> > >>>> Martijn > >>>> > >>>> On Wed, May 13, 2015 at 11:29 PM, Guillaume Smet > >>>> <[email protected]> wrote: > >>>>> I gave it some more thoughts and I admit it's a bit radical but I > think > >>>> we > >>>>> should simply remove all these constructors and give > StringResourceModel > >>>> a > >>>>> fluid interface. > >>>>> > >>>>> We could keep 2 constructors: > >>>>> StringResourceModel(final String resourceKey, final Component > component) > >>>>> StringResourceModel(final String resourceKey) > >>>>> > >>>>> and add the following methods: > >>>>> setDefaultValue > >>>>> setModel > >>>>> setParameters > >>>>> all these methods returning 'this'. > >>>>> > >>>>> We often call the constructors with null values for some of these > >>>>> parameters anyway and it would be far more readable and less error > prone. > >>>>> > >>>>> When I upgrade, I really prefer having compilation errors rather than > >>>> code > >>>>> silently failing (currently it might not take the default value and > it > >>>>> might get an off by one error for the parameters). > >>>>> > >>>>> Thoughts? > >>>>> > >>>>> On Wed, May 13, 2015 at 1:44 PM, Guillaume Smet < > >>>> [email protected]> > >>>>> wrote: > >>>>> > >>>>>> Yup, that's exactly the issue I had. > >>>>>> > >>>>>> It's not hard to fix in user code. The hard part is to find the > >>>>>> occurrences of the issue in the code. > >>>>>> > >>>>>> > >>>>>> On Wed, May 13, 2015 at 8:05 AM, Martin Grigorov < > [email protected]> > >>>>>> wrote: > >>>>>> > >>>>>>> https://issues.apache.org/jira/browse/WICKET-5906 sounds related. > >>>>>>> > >>>>>>> Martin Grigorov > >>>>>>> Wicket Training and Consulting > >>>>>>> https://twitter.com/mtgrigorov > >>>>>>> > >>>>>>> On Mon, May 11, 2015 at 11:26 PM, Guillaume Smet < > >>>>>>> [email protected]> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> Still working on the migration to 7. > >>>>>>>> > >>>>>>>> We used this pattern with Wicket 6: > >>>>>>>> new StringResourceModel(propertyModel.getObject(), null, > >>>>>>>> propertyModel.getObject(), objectModel, secondaryObjectModel)) > >>>>>>>> which was directed to the following constructor: > >>>>>>>> StringResourceModel(key, model, defaultValue, Object... > parameters) > >>>>>>>> > >>>>>>>> When migrating to 7, this pattern is silently directed to the > >>>> following > >>>>>>>> constructor: > >>>>>>>> StringResourceModel(key, model, Object... parameters) > >>>>>>>> thus the replacements are wrong. > >>>>>>>> > >>>>>>>> The fact that our application is silently broken is kinda > annoying. > >>>>>>>> > >>>>>>>> I ended up moving to: > >>>>>>>> new StringResourceModel(propertyModel.getObject(), (IModel<?>) > null, > >>>>>>>> propertyModel, new Object[] { objectModel, secondaryObjectModel > })) > >>>>>>>> > >>>>>>>> I don't know if there is something to do about it but I thought I > >>>> might > >>>>>>> as > >>>>>>>> well get it out here. > >>>>>>>> > >>>>>>>> -- > >>>>>>>> Guillaume > >>>> > >>>> > >>>> > >>>> -- > >>>> Become a Wicket expert, learn from the best: > http://wicketinaction.com > >>>> > > > > > > > > -- > > Become a Wicket expert, learn from the best: http://wicketinaction.com > > > > -- > Become a Wicket expert, learn from the best: http://wicketinaction.com >
