LocaleProviderFactory

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

LocaleProviderFactory

Lukasz Lenart
If no objections I will merge this PR [1] tomorrow

[1] https://github.com/apache/struts/pull/122


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: LocaleProviderFactory

Stefaan Dutry-2
I have a question about the pull request.

In the interface LocaleProviderFactory, there's only a method called
createLocaleProvider.

Does this mean that there's supposed to be a seperate instance of
LocaleProvider on every single place this is used?
(I know you could technicaly implement the interface and provide the
exact same provider, but seeing the method is named create and not get
(or obtain or something) it sounds to me that it's the intention to
return a new instance every single time)

What is the reason that this object (the LocaleProvider) should not be reused?
Is it because ActionSupport still implements LocaleProvider?
This doesn't realy prevent that an ActionSupport instance gets used as
LocaleProvider?

Please excuse my ignorance if this is a dumb question.

Regards,

Stefaan Dutry (sdutry)

2017-03-15 18:54 GMT+01:00 Lukasz Lenart <[hidden email]>:

> If no objections I will merge this PR [1] tomorrow
>
> [1] https://github.com/apache/struts/pull/122
>
>
> Regards
> --
> Łukasz
> + 48 606 323 122 http://www.lenart.org.pl/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: LocaleProviderFactory

Lukasz Lenart
2017-03-15 20:01 GMT+01:00 Stefaan Dutry <[hidden email]>:

> I have a question about the pull request.
>
> In the interface LocaleProviderFactory, there's only a method called
> createLocaleProvider.
>
> Does this mean that there's supposed to be a seperate instance of
> LocaleProvider on every single place this is used?
> (I know you could technicaly implement the interface and provide the
> exact same provider, but seeing the method is named create and not get
> (or obtain or something) it sounds to me that it's the intention to
> return a new instance every single time)
>
> What is the reason that this object (the LocaleProvider) should not be reused?
> Is it because ActionSupport still implements LocaleProvider?
> This doesn't realy prevent that an ActionSupport instance gets used as
> LocaleProvider?

Good question ... my intention was to return a new instance because it
is safer, you don't need locks, synchronization, etc. Also
LocaleProvider is a very simple interface with simple implementation.
And as you already noticed it's connected with ActionSupport where
actions are created per each request, which means its ActionContext
will also be re-created each time. And as the default implementation
based on ActionContext, recreating the LocaleProvider makes sense.

Now you get me thinking about usage of the interface in the
I18NInterceptor - a long living object - I think I need to change how
it uses the LocaleProvider.

> Please excuse my ignorance if this is a dumb question.

Nope, there is no dumb questions :) Thank you for your insights :)


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: LocaleProviderFactory

Greg Huber
In reply to this post by Lukasz Lenart
+1, works for me.

Cheers Greg

On 15 March 2017 at 17:54, Lukasz Lenart <[hidden email]> wrote:

> If no objections I will merge this PR [1] tomorrow
>
> [1] https://github.com/apache/struts/pull/122
>
>
> Regards
> --
> Łukasz
> + 48 606 323 122 http://www.lenart.org.pl/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: LocaleProviderFactory

Lukasz Lenart
2017-03-16 10:21 GMT+01:00 Greg Huber <[hidden email]>:
> +1, works for me.

Cool, thanks :) Can you mark the PR with :+1: as well?


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: LocaleProviderFactory

Greg Huber
Would that be in the comment?

On 16 March 2017 at 09:23, Lukasz Lenart <[hidden email]> wrote:

> 2017-03-16 10:21 GMT+01:00 Greg Huber <[hidden email]>:
> > +1, works for me.
>
> Cool, thanks :) Can you mark the PR with :+1: as well?
>
>
> Regards
> --
> Łukasz
> + 48 606 323 122 http://www.lenart.org.pl/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: LocaleProviderFactory

Lukasz Lenart
2017-03-16 10:26 GMT+01:00 Greg Huber <[hidden email]>:
> Would that be in the comment?

Comment or you can mark the PR's description (on the right there is
"Add your reaction" smiley face )


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...