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 Signature for PrettyPrint #573

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Conversation

wildeng
Copy link

@wildeng wildeng commented Jan 10, 2021

Added the prettyprint signature file.
Marked as WIP because I'm trying to write the related tests.

@wildeng
Copy link
Author

wildeng commented Jan 15, 2021

Removing the WIP because I find difficult implementing the tests, although I will try more.

@wildeng wildeng changed the title WIP: Add Signature for PrettyPrint Add Signature for PrettyPrint Jan 15, 2021
@@ -0,0 +1,364 @@
# frozen_string_literal: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RBS doesn't understand frozen_string_literal directive. ✂️

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove it

# output
# end
#
def self.format: (?untyped output, ?Integer maxwidth, ?String newline, ?untyped genspace) ?{ (Integer) -> Integer } -> untyped
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make an interface with << method, and use the interface as the type of output here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I correctly understood something like:

interface  _Output
  def <<: (?String obj, ?String sep, ?String newline, ?untyped genspace ) ?{ (Integer) -> Integer } -> void
end

An then the definition becomes

   def self.format: (?untyped output, ?Integer maxwidth, ?String newline, ?untyped genspace) ?{ (Integer) -> Integer } -> Output

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean like the following:

class PrettyPrint
  interface _Output
    def <<: (String) -> void
  end

  def self.format: (?_Output output, ?Integer maxwidth, ?String newline, ?untyped genspace) ?{ (Integer) -> Integer } -> _Output # !?!?
end

Yes, it's my bad... I finally found that the type cannot be written in RBS... 😢

Leave the type of the first argument ?untyped. 🙏 (I'm sorry for the confusion.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries at all :)

#
# If `width` is not specified, obj.length is used.
#
def text: (untyped obj, ?Integer width) -> untyped
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we need untyped here as the type of obj. Is there any reason to make it other than String?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point, although the documentation is a little bit obscure for me. It's implicit the use of String.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also got confused a bit. 😓
As far as I know, obj is (implicitly) assumed to be a String.


# Breaks the buffer into lines that are shorter than #maxwidth
#
def break_outmost_groups: () -> untyped
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like these -> untyped can be -> void?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely. I will change this

@soutaro
Copy link
Member

soutaro commented Jan 16, 2021

@wildeng Thank you for opening this PR. 👏

I post some comments about possible improvements. And hope this commit 021bb65 helps you writing tests.

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚧

@wildeng
Copy link
Author

wildeng commented Jan 16, 2021

@soutaro thanks a lot for the comments and the test templates. I'm really happy to try to give back what Ruby gave to me in all these years.

@wildeng wildeng force-pushed the add-pretty-print branch 2 times, most recently from 4680fe7 to 3cc2124 Compare January 16, 2021 15:10
@wildeng wildeng marked this pull request as draft January 16, 2021 17:24
@wildeng wildeng marked this pull request as ready for review January 22, 2021 08:54
@wildeng
Copy link
Author

wildeng commented Jan 22, 2021

I've checked the original code and modified the PR accordingly and did the requested changes. I also wrote the tests for it.
I really hope it's better than when I opened it 😄

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 💯
Could you fixup the commits?

  * Added prettyprint.rbs file with definitions and rdoc
    annotations
  * Added the tests
@wildeng
Copy link
Author

wildeng commented Jan 24, 2021

Looks great! 💯
Could you fixup the commits?

Done :)

@soutaro soutaro merged commit f4ef2b7 into ruby:master Jan 27, 2021
@soutaro
Copy link
Member

soutaro commented Jan 27, 2021

Merged! Thank you! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants