pharo-project / pharo

Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk.
http://pharo.org
Other
1.21k stars 356 forks source link

String>>withUnixLineEndings shouldn't modify the the original string #3048

Closed tinchodias closed 5 years ago

tinchodias commented 5 years ago

Ronie Salgado reported this issue in pharo-dev mailing list 10 days ago:

Hi,

I just noticed that String >> withUnixLineEndings modifies the the original string, at least when the string only has cr, and not crlf. For example, try the following (in Pharo 7) in a Playground:

original :='asa' , String cr , 'asa'. newString := original withUnixLineEndings. original at: 4.

Best regards, Ronie

aranega commented 5 years ago

@tinchodias we checked very quickly, and we are not sure of what should be the behavior of the #withUnixLineEnding method. Indeed, currently, the modification is made in-place, but there's nothing in the documentation of the method that states it should not. What would be the right behavior for this method?

tinchodias commented 5 years ago

Hi @aranega, thanks for answering.

I checked briefly if there was something in Pharo with Style to reference but didn't find it quickly.

One concrete argument is that other selectors that begin with with don't mutate self. At least, when I briefly inspected in Pharo 8 didn't find any (I usedSystemNavigation default allMethods select: [ :each | each selector beginsWith: 'with' ]).

Trying to be less concrete (more abstract), I guess the rule is that methods that begin with prepositions don't mutate. Often mutators without arguments begin with 'be', for example ZnClient>>beBinary, and actually don't remember other beginnings for this case.

So... I'd propose to either rename to beUnixLineEndings or to make withUnixLineEnding answer a new string. I vote for 2nd one.

tinchodias commented 5 years ago

@ronsaldo

ronsaldo commented 5 years ago

I also vote for the copying behavior. For me it makes more sense.

svenvc commented 5 years ago

I vote for the second option

withXYZ: should copy

stale[bot] commented 5 years ago

To limit bug bankruptcy (see https://www.joelonsoftware.com/2012/07/09/software-inventory/) this issue has been automatically marked as stale because it has not had any activity in 6 months. It will be closed in 1 month if no further activity occurs. If this issue remains important to you, please comment to reactivate the issue. Thank you for your contributions.

Joel on Software
Software Inventory
Imagine, for a moment, that you came upon a bread factory for the first time. At first it just looks like a jumble of incomprehensible machinery with a few people buzzing around. As your eyes adjus…