[csv] Fixes for commons-csv

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[csv] Fixes for commons-csv

Jacopo Cappellato
Hi all,

I am a committer of the Apache OFBiz project and I am using commons-csv for a project at work.
I am wondering if there are plans to maintain the common-csv package; if I will submit some patches and small enhancements will they be considered?
If there is interest around this, I  may also be available to help to resolve some of the Jira task listed here:

https://issues.apache.org/jira/browse/SANDBOX/fixforversion/12313514

in order to be able to issue a release.

Kind regards,

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

Reply | Threaded
Open this post in threaded view
|

RE: [csv] Fixes for commons-csv

Gary Gregory
Hi Jacopo,

First, yes, please contribute updates and fixes.

Would you be willing to contribute more than 'small' things to turn the project into a real 1.0 release?

I've not looked at the component in a loooong time so I am not sure what would be needed.

Gary Gregory
Senior Software Engineer
Rocket Software
3340 Peachtree Road, Suite 820 . Atlanta, GA 30326 . USA
Tel: +1.404.760.1560
Email: [hidden email]
Web: seagull.rocketsoftware.com 



> -----Original Message-----
> From: Jacopo Cappellato [mailto:[hidden email]]
> Sent: Friday, January 28, 2011 11:14
> To: [hidden email]
> Subject: [csv] Fixes for commons-csv
>
> Hi all,
>
> I am a committer of the Apache OFBiz project and I am using commons-csv for
> a project at work.
> I am wondering if there are plans to maintain the common-csv package; if I
> will submit some patches and small enhancements will they be considered?
> If there is interest around this, I  may also be available to help to
> resolve some of the Jira task listed here:
>
> https://issues.apache.org/jira/browse/SANDBOX/fixforversion/12313514
>
> in order to be able to issue a release.
>
> Kind regards,
>
> Jacopo
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Fixes for commons-csv

Jacopo Cappellato
Hi Gary,

first of all, thanks for the quick response; please see my comments inline:

On Jan 28, 2011, at 5:23 PM, Gary Gregory wrote:

> Hi Jacopo,
>
> First, yes, please contribute updates and fixes.
>

Great, will do.

> Would you be willing to contribute more than 'small' things to turn the project into a real 1.0 release?

I think that the answer is "yes": we are using a snapshot for a project here at work and it would be great if we could use a real release; I would be happy to help in the process.

Kind regards,

Jacopo

>
> I've not looked at the component in a loooong time so I am not sure what would be needed.
>
> Gary Gregory
> Senior Software Engineer
> Rocket Software
> 3340 Peachtree Road, Suite 820 . Atlanta, GA 30326 . USA
> Tel: +1.404.760.1560
> Email: [hidden email]
> Web: seagull.rocketsoftware.com  
>
>
>
>> -----Original Message-----
>> From: Jacopo Cappellato [mailto:[hidden email]]
>> Sent: Friday, January 28, 2011 11:14
>> To: [hidden email]
>> Subject: [csv] Fixes for commons-csv
>>
>> Hi all,
>>
>> I am a committer of the Apache OFBiz project and I am using commons-csv for
>> a project at work.
>> I am wondering if there are plans to maintain the common-csv package; if I
>> will submit some patches and small enhancements will they be considered?
>> If there is interest around this, I  may also be available to help to
>> resolve some of the Jira task listed here:
>>
>> https://issues.apache.org/jira/browse/SANDBOX/fixforversion/12313514
>>
>> in order to be able to issue a release.
>>
>> Kind regards,
>>
>> Jacopo
>> ---------------------------------------------------------------------
>> 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]
>


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

Reply | Threaded
Open this post in threaded view
|

Re: [csv] Fixes for commons-csv

Simone Tripodi-2
Hi all guys,
count also on my help if needed, I can help on applying patches and
for the release process.
Looking forward for news,
Simo

http://people.apache.org/~simonetripodi/
http://www.99soft.org/



On Fri, Jan 28, 2011 at 6:54 PM, Jacopo Cappellato
<[hidden email]> wrote:

> Hi Gary,
>
> first of all, thanks for the quick response; please see my comments inline:
>
> On Jan 28, 2011, at 5:23 PM, Gary Gregory wrote:
>
>> Hi Jacopo,
>>
>> First, yes, please contribute updates and fixes.
>>
>
> Great, will do.
>
>> Would you be willing to contribute more than 'small' things to turn the project into a real 1.0 release?
>
> I think that the answer is "yes": we are using a snapshot for a project here at work and it would be great if we could use a real release; I would be happy to help in the process.
>
> Kind regards,
>
> Jacopo
>
>>
>> I've not looked at the component in a loooong time so I am not sure what would be needed.
>>
>> Gary Gregory
>> Senior Software Engineer
>> Rocket Software
>> 3340 Peachtree Road, Suite 820 . Atlanta, GA 30326 . USA
>> Tel: +1.404.760.1560
>> Email: [hidden email]
>> Web: seagull.rocketsoftware.com
>>
>>
>>
>>> -----Original Message-----
>>> From: Jacopo Cappellato [mailto:[hidden email]]
>>> Sent: Friday, January 28, 2011 11:14
>>> To: [hidden email]
>>> Subject: [csv] Fixes for commons-csv
>>>
>>> Hi all,
>>>
>>> I am a committer of the Apache OFBiz project and I am using commons-csv for
>>> a project at work.
>>> I am wondering if there are plans to maintain the common-csv package; if I
>>> will submit some patches and small enhancements will they be considered?
>>> If there is interest around this, I  may also be available to help to
>>> resolve some of the Jira task listed here:
>>>
>>> https://issues.apache.org/jira/browse/SANDBOX/fixforversion/12313514
>>>
>>> in order to be able to issue a release.
>>>
>>> Kind regards,
>>>
>>> Jacopo
>>> ---------------------------------------------------------------------
>>> 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]
>>
>
>
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Fixes for commons-csv

Jacopo Cappellato
Thank you Simone!

Ciao

Jacopo

On Jan 28, 2011, at 8:39 PM, Simone Tripodi wrote:

> Hi all guys,
> count also on my help if needed, I can help on applying patches and
> for the release process.
> Looking forward for news,
> Simo
>
> http://people.apache.org/~simonetripodi/
> http://www.99soft.org/
>
>
>
> On Fri, Jan 28, 2011 at 6:54 PM, Jacopo Cappellato
> <[hidden email]> wrote:
>> Hi Gary,
>>
>> first of all, thanks for the quick response; please see my comments inline:
>>
>> On Jan 28, 2011, at 5:23 PM, Gary Gregory wrote:
>>
>>> Hi Jacopo,
>>>
>>> First, yes, please contribute updates and fixes.
>>>
>>
>> Great, will do.
>>
>>> Would you be willing to contribute more than 'small' things to turn the project into a real 1.0 release?
>>
>> I think that the answer is "yes": we are using a snapshot for a project here at work and it would be great if we could use a real release; I would be happy to help in the process.
>>
>> Kind regards,
>>
>> Jacopo
>>
>>>
>>> I've not looked at the component in a loooong time so I am not sure what would be needed.
>>>
>>> Gary Gregory
>>> Senior Software Engineer
>>> Rocket Software
>>> 3340 Peachtree Road, Suite 820 . Atlanta, GA 30326 . USA
>>> Tel: +1.404.760.1560
>>> Email: [hidden email]
>>> Web: seagull.rocketsoftware.com
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jacopo Cappellato [mailto:[hidden email]]
>>>> Sent: Friday, January 28, 2011 11:14
>>>> To: [hidden email]
>>>> Subject: [csv] Fixes for commons-csv
>>>>
>>>> Hi all,
>>>>
>>>> I am a committer of the Apache OFBiz project and I am using commons-csv for
>>>> a project at work.
>>>> I am wondering if there are plans to maintain the common-csv package; if I
>>>> will submit some patches and small enhancements will they be considered?
>>>> If there is interest around this, I  may also be available to help to
>>>> resolve some of the Jira task listed here:
>>>>
>>>> https://issues.apache.org/jira/browse/SANDBOX/fixforversion/12313514
>>>>
>>>> in order to be able to issue a release.
>>>>
>>>> Kind regards,
>>>>
>>>> Jacopo
>>>> ---------------------------------------------------------------------
>>>> 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]
>>>
>>
>>
>> ---------------------------------------------------------------------
>> 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]
>


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

Reply | Threaded
Open this post in threaded view
|

[csv] Proposal to remove setter methods from CSVStrategy

Jacopo Cappellato
In reply to this post by Gary Gregory
Hi all,

in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):

    public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
                                                                 true, false, true);
    public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
                                                                 false, false, false);
    public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
                                                                 true, false, true);

What do you think?

Jacopo


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

Reply | Threaded
Open this post in threaded view
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

jodastephen
Traditionally, many commons projects has taken no position on the
correct way to instantiate objects, whether via constructor or bean
methods. This was to allow construction from tools such as Velocity.
It is also still easier to construct using Spring via bean methods
rather than the constructor.

These days, immutability and concurrency are more important, so this
change may be applicable, buts its good to understand the history and
wide use cases of classes like these.

Stephen


On 29 January 2011 12:04, Jacopo Cappellato <[hidden email]> wrote:

> Hi all,
>
> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>
>    public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>                                                                 true, false, true);
>    public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>                                                                 false, false, false);
>    public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>                                                                 true, false, true);
>
> What do you think?
>
> Jacopo
>
>
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Fixes for commons-csv

Jörg Schaible
In reply to this post by Jacopo Cappellato
Hi Jacopo,

Jacopo Cappellato wrote:

> Hi all,
>
> I am a committer of the Apache OFBiz project and I am using commons-csv
> for a project at work. I am wondering if there are plans to maintain the
> common-csv package; if I will submit some patches and small enhancements
> will they be considered?

Actually any ASF committer can have karma for the sandbox here in Commons
(see "The Sandbox" in http://wiki.apache.org/commons/CommonsEtiquette).
Simply ask for it :)

It's always preferred to keep track of changes for a component using JIRA
and discuss design or restructurings here on the list though.

> If there is interest around this, I  may also be available to help to
> resolve some of the Jira task listed here:
>
> https://issues.apache.org/jira/browse/SANDBOX/fixforversion/12313514
>
> in order to be able to issue a release.

Sounds good.

- Jörg


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

Reply | Threaded
Open this post in threaded view
|

Re: [csv] Fixes for commons-csv

Jacopo Cappellato
Thank you Jörg.

At this point I would be happy to get karma for the sandbox (my account is jacopoc): of course I will be very careful about my commits and I will discuss any ideas here before.

Kind regards,

Jacopo


On Jan 29, 2011, at 1:37 PM, Jörg Schaible wrote:

> Hi Jacopo,
>
> Jacopo Cappellato wrote:
>
>> Hi all,
>>
>> I am a committer of the Apache OFBiz project and I am using commons-csv
>> for a project at work. I am wondering if there are plans to maintain the
>> common-csv package; if I will submit some patches and small enhancements
>> will they be considered?
>
> Actually any ASF committer can have karma for the sandbox here in Commons
> (see "The Sandbox" in http://wiki.apache.org/commons/CommonsEtiquette).
> Simply ask for it :)
>
> It's always preferred to keep track of changes for a component using JIRA
> and discuss design or restructurings here on the list though.
>
>> If there is interest around this, I  may also be available to help to
>> resolve some of the Jira task listed here:
>>
>> https://issues.apache.org/jira/browse/SANDBOX/fixforversion/12313514
>>
>> in order to be able to issue a release.
>
> Sounds good.
>
> - Jörg
>
>
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

Jacopo Cappellato
In reply to this post by jodastephen
Thank you Stephen,

I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.

Jacopo

On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:

> Traditionally, many commons projects has taken no position on the
> correct way to instantiate objects, whether via constructor or bean
> methods. This was to allow construction from tools such as Velocity.
> It is also still easier to construct using Spring via bean methods
> rather than the constructor.
>
> These days, immutability and concurrency are more important, so this
> change may be applicable, buts its good to understand the history and
> wide use cases of classes like these.
>
> Stephen
>
>
> On 29 January 2011 12:04, Jacopo Cappellato <[hidden email]> wrote:
>> Hi all,
>>
>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>
>>    public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>                                                                 true, false, true);
>>    public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>                                                                 false, false, false);
>>    public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>                                                                 true, false, true);
>>
>> What do you think?
>>
>> Jacopo
>>
>>
>> ---------------------------------------------------------------------
>> 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]
>


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

Reply | Threaded
Open this post in threaded view
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

Simone Tripodi-2
Hi all guys!!! :)
I'd suggest to apply Jacopo's suggestion, making strategies' fields as
final would help classes becoming thread-safe; to fix the issue of
construction described by Stephen, it would be useful adding builder
classes (optionally implemented as static inner classes).
WDYT? HTH, that's just my opinion,
Simo

http://people.apache.org/~simonetripodi/
http://www.99soft.org/



On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
<[hidden email]> wrote:

> Thank you Stephen,
>
> I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.
>
> Jacopo
>
> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>
>> Traditionally, many commons projects has taken no position on the
>> correct way to instantiate objects, whether via constructor or bean
>> methods. This was to allow construction from tools such as Velocity.
>> It is also still easier to construct using Spring via bean methods
>> rather than the constructor.
>>
>> These days, immutability and concurrency are more important, so this
>> change may be applicable, buts its good to understand the history and
>> wide use cases of classes like these.
>>
>> Stephen
>>
>>
>> On 29 January 2011 12:04, Jacopo Cappellato <[hidden email]> wrote:
>>> Hi all,
>>>
>>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>
>>>    public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>                                                                 true, false, true);
>>>    public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>                                                                 false, false, false);
>>>    public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>                                                                 true, false, true);
>>>
>>> What do you think?
>>>
>>> Jacopo
>>>
>>>
>>> ---------------------------------------------------------------------
>>> 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]
>>
>
>
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

Gary Gregory
Are people really using these classes in an MT way?  These seem like the kind of classes you do not share between threads. My op only though.

Gary

On Jan 29, 2011, at 12:27, "Simone Tripodi" <[hidden email]> wrote:

> Hi all guys!!! :)
> I'd suggest to apply Jacopo's suggestion, making strategies' fields as
> final would help classes becoming thread-safe; to fix the issue of
> construction described by Stephen, it would be useful adding builder
> classes (optionally implemented as static inner classes).
> WDYT? HTH, that's just my opinion,
> Simo
>
> http://people.apache.org/~simonetripodi/
> http://www.99soft.org/
>
>
>
> On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
> <[hidden email]> wrote:
>> Thank you Stephen,
>>
>> I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.
>>
>> Jacopo
>>
>> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>>
>>> Traditionally, many commons projects has taken no position on the
>>> correct way to instantiate objects, whether via constructor or bean
>>> methods. This was to allow construction from tools such as Velocity.
>>> It is also still easier to construct using Spring via bean methods
>>> rather than the constructor.
>>>
>>> These days, immutability and concurrency are more important, so this
>>> change may be applicable, buts its good to understand the history and
>>> wide use cases of classes like these.
>>>
>>> Stephen
>>>
>>>
>>> On 29 January 2011 12:04, Jacopo Cappellato <[hidden email]> wrote:
>>>> Hi all,
>>>>
>>>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>>>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>>
>>>>    public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>                                                                 true, false, true);
>>>>    public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>>                                                                 false, false, false);
>>>>    public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>                                                                 true, false, true);
>>>>
>>>> What do you think?
>>>>
>>>> Jacopo
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> 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]
>>>
>>
>>
>> ---------------------------------------------------------------------
>> 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]
>

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

Reply | Threaded
Open this post in threaded view
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

Simone Tripodi-2
indeed, if not, it doesn't worth the value changing the actual design :P
Have a nice WE!
Simo

http://people.apache.org/~simonetripodi/
http://www.99soft.org/



On Sat, Jan 29, 2011 at 9:49 PM, Gary Gregory
<[hidden email]> wrote:

> Are people really using these classes in an MT way?  These seem like the kind of classes you do not share between threads. My op only though.
>
> Gary
>
> On Jan 29, 2011, at 12:27, "Simone Tripodi" <[hidden email]> wrote:
>
>> Hi all guys!!! :)
>> I'd suggest to apply Jacopo's suggestion, making strategies' fields as
>> final would help classes becoming thread-safe; to fix the issue of
>> construction described by Stephen, it would be useful adding builder
>> classes (optionally implemented as static inner classes).
>> WDYT? HTH, that's just my opinion,
>> Simo
>>
>> http://people.apache.org/~simonetripodi/
>> http://www.99soft.org/
>>
>>
>>
>> On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
>> <[hidden email]> wrote:
>>> Thank you Stephen,
>>>
>>> I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.
>>>
>>> Jacopo
>>>
>>> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>>>
>>>> Traditionally, many commons projects has taken no position on the
>>>> correct way to instantiate objects, whether via constructor or bean
>>>> methods. This was to allow construction from tools such as Velocity.
>>>> It is also still easier to construct using Spring via bean methods
>>>> rather than the constructor.
>>>>
>>>> These days, immutability and concurrency are more important, so this
>>>> change may be applicable, buts its good to understand the history and
>>>> wide use cases of classes like these.
>>>>
>>>> Stephen
>>>>
>>>>
>>>> On 29 January 2011 12:04, Jacopo Cappellato <[hidden email]> wrote:
>>>>> Hi all,
>>>>>
>>>>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>>>>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>>>
>>>>>    public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>                                                                 true, false, true);
>>>>>    public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>>>                                                                 false, false, false);
>>>>>    public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>                                                                 true, false, true);
>>>>>
>>>>> What do you think?
>>>>>
>>>>> Jacopo
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> 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]
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> 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]
>>
>
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

Jacopo Cappellato
In reply to this post by Gary Gregory
Hi Gary,

I initially proposed the change when I noticed the following code in one of the automated tests of common-csv:

    CSVStrategy strategy = (CSVStrategy)CSVStrategy.DEFAULT_STRATEGY.clone();
    strategy.setCommentStart('#');
    TestCSVParser parser = new TestCSVParser(new StringReader(code), strategy);

and I realized the importance, in a calling method that alters the default settings, of using "clone".
But apart from this I don't have real world use cases that describe issues around this.

Jacopo


On Jan 29, 2011, at 9:49 PM, Gary Gregory wrote:

> Are people really using these classes in an MT way?  These seem like the kind of classes you do not share between threads. My op only though.
>
> Gary
>
> On Jan 29, 2011, at 12:27, "Simone Tripodi" <[hidden email]> wrote:
>
>> Hi all guys!!! :)
>> I'd suggest to apply Jacopo's suggestion, making strategies' fields as
>> final would help classes becoming thread-safe; to fix the issue of
>> construction described by Stephen, it would be useful adding builder
>> classes (optionally implemented as static inner classes).
>> WDYT? HTH, that's just my opinion,
>> Simo
>>
>> http://people.apache.org/~simonetripodi/
>> http://www.99soft.org/
>>
>>
>>
>> On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
>> <[hidden email]> wrote:
>>> Thank you Stephen,
>>>
>>> I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.
>>>
>>> Jacopo
>>>
>>> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>>>
>>>> Traditionally, many commons projects has taken no position on the
>>>> correct way to instantiate objects, whether via constructor or bean
>>>> methods. This was to allow construction from tools such as Velocity.
>>>> It is also still easier to construct using Spring via bean methods
>>>> rather than the constructor.
>>>>
>>>> These days, immutability and concurrency are more important, so this
>>>> change may be applicable, buts its good to understand the history and
>>>> wide use cases of classes like these.
>>>>
>>>> Stephen
>>>>
>>>>
>>>> On 29 January 2011 12:04, Jacopo Cappellato <[hidden email]> wrote:
>>>>> Hi all,
>>>>>
>>>>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>>>>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>>>
>>>>>   public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>                                                                true, false, true);
>>>>>   public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>>>                                                                false, false, false);
>>>>>   public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>                                                                true, false, true);
>>>>>
>>>>> What do you think?
>>>>>
>>>>> Jacopo
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> 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]
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> 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]
>>
>
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

Adrian Crum-3
 From my perspective, if DEFAULT_STRATEGY, EXCEL_STRATEGY, or TDF_STRATEGY are mutable, then copies of them should be returned in a factory method. Ideally, a selector would be passed to a factory method:

CSVStrategy strategy = CSVStrategy.getInstance(CSVStrategy.DEFAULT_STRATEGY);


-Adrian


On 1/29/2011 8:51 PM, Jacopo Cappellato wrote:

> Hi Gary,
>
> I initially proposed the change when I noticed the following code in one of the automated tests of common-csv:
>
>      CSVStrategy strategy = (CSVStrategy)CSVStrategy.DEFAULT_STRATEGY.clone();
>      strategy.setCommentStart('#');
>      TestCSVParser parser = new TestCSVParser(new StringReader(code), strategy);
>
> and I realized the importance, in a calling method that alters the default settings, of using "clone".
> But apart from this I don't have real world use cases that describe issues around this.
>
> Jacopo
>
>
> On Jan 29, 2011, at 9:49 PM, Gary Gregory wrote:
>
>> Are people really using these classes in an MT way?  These seem like the kind of classes you do not share between threads. My op only though.
>>
>> Gary
>>
>> On Jan 29, 2011, at 12:27, "Simone Tripodi"<[hidden email]>  wrote:
>>
>>> Hi all guys!!! :)
>>> I'd suggest to apply Jacopo's suggestion, making strategies' fields as
>>> final would help classes becoming thread-safe; to fix the issue of
>>> construction described by Stephen, it would be useful adding builder
>>> classes (optionally implemented as static inner classes).
>>> WDYT? HTH, that's just my opinion,
>>> Simo
>>>
>>> http://people.apache.org/~simonetripodi/
>>> http://www.99soft.org/
>>>
>>>
>>>
>>> On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
>>> <[hidden email]>  wrote:
>>>> Thank you Stephen,
>>>>
>>>> I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.
>>>>
>>>> Jacopo
>>>>
>>>> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>>>>
>>>>> Traditionally, many commons projects has taken no position on the
>>>>> correct way to instantiate objects, whether via constructor or bean
>>>>> methods. This was to allow construction from tools such as Velocity.
>>>>> It is also still easier to construct using Spring via bean methods
>>>>> rather than the constructor.
>>>>>
>>>>> These days, immutability and concurrency are more important, so this
>>>>> change may be applicable, buts its good to understand the history and
>>>>> wide use cases of classes like these.
>>>>>
>>>>> Stephen
>>>>>
>>>>>
>>>>> On 29 January 2011 12:04, Jacopo Cappellato<[hidden email]>  wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>>>>>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>>>>
>>>>>>    public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>                                                                 true, false, true);
>>>>>>    public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>>>>                                                                 false, false, false);
>>>>>>    public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>                                                                 true, false, true);
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Jacopo
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> 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]
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> 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]
>>>
>> ---------------------------------------------------------------------
>> 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]
>

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

Reply | Threaded
Open this post in threaded view
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

Jacopo Cappellato
Hi Adrian,

yes this is what I was thinking when I mentioned the factory method.

Jacopo

On Jan 30, 2011, at 8:06 AM, Adrian Crum wrote:

> From my perspective, if DEFAULT_STRATEGY, EXCEL_STRATEGY, or TDF_STRATEGY are mutable, then copies of them should be returned in a factory method. Ideally, a selector would be passed to a factory method:
>
> CSVStrategy strategy = CSVStrategy.getInstance(CSVStrategy.DEFAULT_STRATEGY);
>
>
> -Adrian
>
>
> On 1/29/2011 8:51 PM, Jacopo Cappellato wrote:
>> Hi Gary,
>>
>> I initially proposed the change when I noticed the following code in one of the automated tests of common-csv:
>>
>>     CSVStrategy strategy = (CSVStrategy)CSVStrategy.DEFAULT_STRATEGY.clone();
>>     strategy.setCommentStart('#');
>>     TestCSVParser parser = new TestCSVParser(new StringReader(code), strategy);
>>
>> and I realized the importance, in a calling method that alters the default settings, of using "clone".
>> But apart from this I don't have real world use cases that describe issues around this.
>>
>> Jacopo
>>
>>
>> On Jan 29, 2011, at 9:49 PM, Gary Gregory wrote:
>>
>>> Are people really using these classes in an MT way?  These seem like the kind of classes you do not share between threads. My op only though.
>>>
>>> Gary
>>>
>>> On Jan 29, 2011, at 12:27, "Simone Tripodi"<[hidden email]>  wrote:
>>>
>>>> Hi all guys!!! :)
>>>> I'd suggest to apply Jacopo's suggestion, making strategies' fields as
>>>> final would help classes becoming thread-safe; to fix the issue of
>>>> construction described by Stephen, it would be useful adding builder
>>>> classes (optionally implemented as static inner classes).
>>>> WDYT? HTH, that's just my opinion,
>>>> Simo
>>>>
>>>> http://people.apache.org/~simonetripodi/
>>>> http://www.99soft.org/
>>>>
>>>>
>>>>
>>>> On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
>>>> <[hidden email]>  wrote:
>>>>> Thank you Stephen,
>>>>>
>>>>> I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.
>>>>>
>>>>> Jacopo
>>>>>
>>>>> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>>>>>
>>>>>> Traditionally, many commons projects has taken no position on the
>>>>>> correct way to instantiate objects, whether via constructor or bean
>>>>>> methods. This was to allow construction from tools such as Velocity.
>>>>>> It is also still easier to construct using Spring via bean methods
>>>>>> rather than the constructor.
>>>>>>
>>>>>> These days, immutability and concurrency are more important, so this
>>>>>> change may be applicable, buts its good to understand the history and
>>>>>> wide use cases of classes like these.
>>>>>>
>>>>>> Stephen
>>>>>>
>>>>>>
>>>>>> On 29 January 2011 12:04, Jacopo Cappellato<[hidden email]>  wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>>>>>>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>>>>>
>>>>>>>   public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>>                                                                true, false, true);
>>>>>>>   public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>>>>>                                                                false, false, false);
>>>>>>>   public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>>                                                                true, false, true);
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>> Jacopo
>>>>>>>
>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> 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]
>>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> 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]
>>>>
>>> ---------------------------------------------------------------------
>>> 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]
>>
>
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

jodastephen
In reply to this post by Jacopo Cappellato
The modern standard for altering immutable classes was laid down by Joda-Time

strategy = strategy.withCommentStart("#");

Really, though the question is whether it shoudl be immutable or not.

Stephen


On 30 January 2011 04:51, Jacopo Cappellato <[hidden email]> wrote:

> Hi Gary,
>
> I initially proposed the change when I noticed the following code in one of the automated tests of common-csv:
>
>    CSVStrategy strategy = (CSVStrategy)CSVStrategy.DEFAULT_STRATEGY.clone();
>    strategy.setCommentStart('#');
>    TestCSVParser parser = new TestCSVParser(new StringReader(code), strategy);
>
> and I realized the importance, in a calling method that alters the default settings, of using "clone".
> But apart from this I don't have real world use cases that describe issues around this.
>
> Jacopo
>
>
> On Jan 29, 2011, at 9:49 PM, Gary Gregory wrote:
>
>> Are people really using these classes in an MT way?  These seem like the kind of classes you do not share between threads. My op only though.
>>
>> Gary
>>
>> On Jan 29, 2011, at 12:27, "Simone Tripodi" <[hidden email]> wrote:
>>
>>> Hi all guys!!! :)
>>> I'd suggest to apply Jacopo's suggestion, making strategies' fields as
>>> final would help classes becoming thread-safe; to fix the issue of
>>> construction described by Stephen, it would be useful adding builder
>>> classes (optionally implemented as static inner classes).
>>> WDYT? HTH, that's just my opinion,
>>> Simo
>>>
>>> http://people.apache.org/~simonetripodi/
>>> http://www.99soft.org/
>>>
>>>
>>>
>>> On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
>>> <[hidden email]> wrote:
>>>> Thank you Stephen,
>>>>
>>>> I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.
>>>>
>>>> Jacopo
>>>>
>>>> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>>>>
>>>>> Traditionally, many commons projects has taken no position on the
>>>>> correct way to instantiate objects, whether via constructor or bean
>>>>> methods. This was to allow construction from tools such as Velocity.
>>>>> It is also still easier to construct using Spring via bean methods
>>>>> rather than the constructor.
>>>>>
>>>>> These days, immutability and concurrency are more important, so this
>>>>> change may be applicable, buts its good to understand the history and
>>>>> wide use cases of classes like these.
>>>>>
>>>>> Stephen
>>>>>
>>>>>
>>>>> On 29 January 2011 12:04, Jacopo Cappellato <[hidden email]> wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>>>>>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>>>>
>>>>>>   public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>                                                                true, false, true);
>>>>>>   public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>>>>                                                                false, false, false);
>>>>>>   public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>                                                                true, false, true);
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Jacopo
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> 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]
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> 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]
>>>
>>
>> ---------------------------------------------------------------------
>> 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]
>
>

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

Reply | Threaded
Open this post in threaded view
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

sebb-2-2
On 30 January 2011 09:09, Stephen Colebourne <[hidden email]> wrote:
> The modern standard for altering immutable classes was laid down by Joda-Time
>
> strategy = strategy.withCommentStart("#");

Not sure I undestand that.

> Really, though the question is whether it shoudl be immutable or not.

+1

Whatever decision is taken needs to be documented:
if the class is intended to be immutable, then this needs to be
documented so future maintainers don't break it, and so users know it
is thread-safe.
If not immutable, then the class thread-safety still needs to be documented.

> Stephen
>
>
> On 30 January 2011 04:51, Jacopo Cappellato <[hidden email]> wrote:
>> Hi Gary,
>>
>> I initially proposed the change when I noticed the following code in one of the automated tests of common-csv:
>>
>>    CSVStrategy strategy = (CSVStrategy)CSVStrategy.DEFAULT_STRATEGY.clone();
>>    strategy.setCommentStart('#');
>>    TestCSVParser parser = new TestCSVParser(new StringReader(code), strategy);
>>
>> and I realized the importance, in a calling method that alters the default settings, of using "clone".
>> But apart from this I don't have real world use cases that describe issues around this.
>>
>> Jacopo
>>
>>
>> On Jan 29, 2011, at 9:49 PM, Gary Gregory wrote:
>>
>>> Are people really using these classes in an MT way?  These seem like the kind of classes you do not share between threads. My op only though.
>>>
>>> Gary
>>>
>>> On Jan 29, 2011, at 12:27, "Simone Tripodi" <[hidden email]> wrote:
>>>
>>>> Hi all guys!!! :)
>>>> I'd suggest to apply Jacopo's suggestion, making strategies' fields as
>>>> final would help classes becoming thread-safe; to fix the issue of
>>>> construction described by Stephen, it would be useful adding builder
>>>> classes (optionally implemented as static inner classes).
>>>> WDYT? HTH, that's just my opinion,
>>>> Simo
>>>>
>>>> http://people.apache.org/~simonetripodi/
>>>> http://www.99soft.org/
>>>>
>>>>
>>>>
>>>> On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
>>>> <[hidden email]> wrote:
>>>>> Thank you Stephen,
>>>>>
>>>>> I understand this; alternatively we could implement a factory method that clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any decision, it is not a big deal for me and we can wait for additional feedback.
>>>>>
>>>>> Jacopo
>>>>>
>>>>> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>>>>>
>>>>>> Traditionally, many commons projects has taken no position on the
>>>>>> correct way to instantiate objects, whether via constructor or bean
>>>>>> methods. This was to allow construction from tools such as Velocity.
>>>>>> It is also still easier to construct using Spring via bean methods
>>>>>> rather than the constructor.
>>>>>>
>>>>>> These days, immutability and concurrency are more important, so this
>>>>>> change may be applicable, buts its good to understand the history and
>>>>>> wide use cases of classes like these.
>>>>>>
>>>>>> Stephen
>>>>>>
>>>>>>
>>>>>> On 29 January 2011 12:04, Jacopo Cappellato <[hidden email]> wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> in my opinion all the setter methods should be removed from the CSVStrategy class: in this way the fields will only be set using the constructors and they will become readonly.
>>>>>>> The main issue I see with the current implementation is that a calling method can modify the values of the fields of the following static objects declared in CSVStrategy (changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>>>>>
>>>>>>>   public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>>                                                                true, false, true);
>>>>>>>   public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>>>>>                                                                false, false, false);
>>>>>>>   public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t', '"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>>                                                                true, false, true);
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>> Jacopo
>>>>>>>
>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> 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]
>>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> 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]
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> 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]
>>
>>
>
> ---------------------------------------------------------------------
> 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
|

Re: [csv] Fixes for commons-csv

Jörg Schaible
In reply to this post by Jacopo Cappellato
Hi Jacopo,

Jacopo Cappellato wrote:

> Thank you Jörg.
>
> At this point I would be happy to get karma for the sandbox (my account is
> jacopoc): of course I will be very careful about my commits and I will
> discuss any ideas here before.

You have karma now.

Cheers,
Jörg


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

Reply | Threaded
Open this post in threaded view
|

Re: [csv] Proposal to remove setter methods from CSVStrategy

jodastephen
In reply to this post by sebb-2-2
On 30 January 2011 15:11, sebb <[hidden email]> wrote:
> On 30 January 2011 09:09, Stephen Colebourne <[hidden email]> wrote:
>> The modern standard for altering immutable classes was laid down by Joda-Time
>>
>> strategy = strategy.withCommentStart("#");
>
> Not sure I undestand that.

An immutable setter, as in
http://joda-time.sourceforge.net/api-release/org/joda/time/DateTime.html#withYear%28int%29

Stephen

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

12