Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new #printOneLineString and related methods #4854

Conversation

@leonardoce
Copy link
Contributor

leonardoce commented Oct 7, 2019

It's nice to have a #printOneLineString that is just like #printString
but avoids printing CRs and LFs. This patch implements such idea adding
a method named #printOnOneLine:, working on a Stream, and a
FilteringStream.

The FilteringStream is used to replace CRs and LFs with a space, but can
be used to filter elements going into a Stream via a generic block.

Fixes: #4849

@Ducasse Ducasse requested a review from estebanlm Oct 7, 2019
@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 7, 2019

Nice :) You were fast. And with tests. Excellent. I could not do it better :)
BTW I think that your tests do not cover the case for lf (or I did not see it) because Pharo internally is using cr (unfortunately).

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 7, 2019

Rereading the code I have some suggestion:

  • first
    =========================================================
    Object >> printOnOneLine: aStream [
    "Append to the argument, aStream, a sequence of characters that
    identifies the receiver, while replacing cr and lf with one space"

    |filter spaces|
    spaces := Set with: Character cr with: Character lf.
    filter := FilteringStream on: aStream with: [ :c |
    (spaces includes: c) ifTrue:Character space ifFalse:c ].
    self printOn: filter.
    ]

I would write this way because ifTrue:ifFalse: requires blocks.

Object >> printOnOneLine: aStream [
"Append to the argument, aStream, a sequence of characters that
identifies the receiver, while replacing cr and lf with one space"

| filter spaces |
spaces := { Character cr . Character lf}. 
filter := FilteringStream on: aStream with: [ :c | 
	(spaces includes: c) ifTrue: [ Character space ] ifFalse: [c] ].
self printOn: filter.

]

2

With your implementation subclasses are forced to do a super printOnOneLine:

I would better do it like

printOnOneLineString [
"Answer a String whose characters are a description of the receiver,
avoiding printing cr and lf"

| filter spaces |
spaces := { Character cr . Character lf}. 
filter := FilteringStream on: aStream with: [ :c | 
	(spaces includes: c) ifTrue: [ Character space ] ifFalse: [c] ].
self printOnOneLine: filter.
    ^ filter contents

Object >> printOnOneLine: aStream [
"Append to the argument, aStream, a sequence of characters that
identifies the receiver, while replacing cr and lf with one space"

   self printOn: aStream

This way the extended can only subclass printOn: and if needed printOnOneLine:
but he is not forced to do a super call.

What do you think?

@leonardoce

This comment has been minimized.

Copy link
Contributor Author

leonardoce commented Oct 7, 2019

You too were really fast and helpful with your review! Thank you for that.

[...]
BTW I think that your tests do not cover the case for lf (or I did not see it) because Pharo internally is using cr (unfortunately).

Oh, I didn't realize that. I'll cover also lf in the tests and modify my commit accordingly.

Rereading the code I have some suggestion:
* first
[...] I would write this way because ifTrue:ifFalse: requires blocks. [...]

Totally agree with that, I'll modify my patch.

2

[...]
Object >> printOnOneLine: aStream [
"Append to the argument, aStream, a sequence of characters that
identifies the receiver, while replacing cr and lf with one space"

   self printOn: aStream

This way the extended can only subclass printOn: and if needed printOnOneLine:
but he is not forced to do a super call.

I'm surely wrong, but at first glance with your proposal #printOnOneLine: and #printOn: have exactly the same behavior, and that means that #printOnOneLine: wouldn't replace cr and lf with spaces.

What I misunderstood? :) Thanks!

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 7, 2019

My idea is that printOnOneLineString is the template method and printOnOneLineOn: is a hook (whose default is just printOn: with the eating new line stream).

printOnOneLineOn: will not have the same behavior since it is using a different stream than printOn:. But what is beautiful with this design is that we can still fully reuse the logic of the printOn: for free (we just changed the behavior of the stream but reuse the logic of printing).

This is super nice.

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 7, 2019

Is it better?
Julien proposed the idea and I love it, so nice.

@leonardoce

This comment has been minimized.

Copy link
Contributor Author

leonardoce commented Oct 7, 2019

Now I understand! That's a good idea.
I'll modify my patch including your considerations.

@leonardoce leonardoce force-pushed the leonardoce:4849-Introduce-ObjectprintOneLineString branch from 1c41471 to 11b787d Oct 7, 2019
It's nice to have a #printOneLineString that is just like #printString
but avoids printing CRs and LFs. This patch implements such idea adding
also a method named #printOnOneLine: which will get called with a
FilteringStream implementing the required behaviour.

The FilteringStream is used to replace CRs and LFs with a space, but can
also be used to filter elements going into a Stream via a generic block.

Fixes: #4849
@leonardoce leonardoce force-pushed the leonardoce:4849-Introduce-ObjectprintOneLineString branch from 11b787d to 862798c Oct 7, 2019
@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 8, 2019

This looks good to me.

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 8, 2019

I will give a chance to others to have a look.

@leonardoce

This comment has been minimized.

Copy link
Contributor Author

leonardoce commented Oct 8, 2019

Thank you @Ducasse!

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 8, 2019

BTW I love your comments!
Everybody should learn from them!.
I entered other issues related to printString and printOn: thinking that you could be interested.

@Ducasse Ducasse requested review from Ducasse and removed request for estebanlm Oct 8, 2019
@leonardoce

This comment has been minimized.

Copy link
Contributor Author

leonardoce commented Oct 8, 2019

@Ducasse, you are too kind to me. I'm reading the issues you created.

@Ducasse Ducasse merged commit daa7cb1 into pharo-project:Pharo8.0 Oct 11, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
probot/minimum-reviews Pending review approvals
WIP Ready for review
Details
@MarcusDenker

This comment has been minimized.

Copy link
Member

MarcusDenker commented Oct 11, 2019

The build fails after this was integrated.

@MarcusDenker

This comment has been minimized.

Copy link
Member

MarcusDenker commented Oct 11, 2019

I think we need to revert this for now

@MarcusDenker

This comment has been minimized.

Copy link
Member

MarcusDenker commented Oct 11, 2019

I create a revert PR. I do not yet understand it... after this PR was merged, the main build failed, with something related to printing. But it looks strange.

'Errors in script loaded from /builds/workspace/est_and_branch_Pipeline_Pharo8.0/32/bootstrap/scripts/bootstrap.st'
[32] MessageNotUnderstood: receiver of "definition" is nil
[32] UndefinedObject(Object)>>doesNotUnderstand: #definition
[32] PBSpurClassLoader(PBClassLoader)>>executeDefinitionFor:
[32] PBSpurClassLoader(PBClassLoader)>>createBehaviorFromDefinition:
[32] PBImageBuilderSpur5032bit(PBImageBuilder50)>>createBehaviorFromDefinition:
[32] [ ^ super createBehaviorFromDefinition: aRFiDefinition ] in PBImageBuilderSpur5032bit(PBImageBuilderSpur50)>>createBehaviorFromDefinition: in Block: [ ^ super createBehaviorFromDefinition: aRFiDefini...etc...
[32] BlockClosure>>on:do:
[32] PBImageBuilderSpur5032bit(PBImageBuilderSpur50)>>createBehaviorFromDefinition:
[32] EPASTInterpreter>>loadClass:
[32] EPASTClassLoaderVisitor>>visitVariableNode:
[32] RBVariableNode>>acceptVisitor:
[32] EPASTInterpreter>>visitClassDefinition:
[32] EPASTClassDefinition>>acceptVisitor:
[32] EPASTInterpreter(ASTInterpreter)>>interpret:
[32] EPASTInterpreter>>evaluateAST:
[32] EPASTEvaluator>>evaluateAST:
[32] EPASTEvaluator>>evaluateCode:withTemps:
[32] EPASTEvaluator>>evaluateCode:
[32] PBSpurClassLoader(PBClassLoader)>>executeDefinitionFor:
[32] PBSpurClassLoader(PBClassLoader)>>createBehaviorFromDefinition:

@MarcusDenker

This comment has been minimized.

Copy link
Member

MarcusDenker commented Oct 11, 2019

[32] *** Warning: Deprecation: The method Object>>#name called from PBClassLoader>>#createBehaviorFromDefinition: has been deprecated.
[32] Implement your own domain representation of an object, or use #asString or #printString instead.

@MarcusDenker

This comment has been minimized.

Copy link
Member

MarcusDenker commented Oct 11, 2019

I think the reason is that the Test case is part of the Streams package, so the boo trap loads it but the superclass (TestCase) is not there.

The bootstrap works again after reverting.

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 12, 2019

What I do not understand is why the builds worked in the first place. I will have a look

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 14, 2019

I'm confused. Now the status says merged but I would like to get this issue still open.
Because it is not integrated.

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 14, 2019

@tesonep I would really like to sit with you to

  • understand why the validation did not bark before
  • understand how I can reopen this issue :)
  • fix it :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.