Skip to content

Conversation

@stackmystack
Copy link
Collaborator

Addresses many points in #55

@stackmystack
Copy link
Collaborator Author

  1. I caught a couple of issues with separate.
  2. Surround is still not implemented
  3. We should not forget to add ruby 3.0+ to the CI.

Comment on lines +212 to +213
case delim
in nil then ['', '']
in String | Symbol then [delim, delim]
in Array then delim.values_at(0, 1).map(&:to_s)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should extract this part to mixins since we have meen using it twice.

space = ->(n) { '---' * n }

printer = Oppen::Wadler.new(space: space)
printer = Oppen::Wadler.new(indent: 2, space: space)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

Comment on lines +8 to +9
printer_width_default = Oppen::Wadler.new(indent: 2)
printer_width_narrow = Oppen::Wadler.new(indent: 2, width: width)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

Comment on lines 7 to 9
# @return [Integer]
# the starting indentation level for the whole printer.
attr_reader :base_indent
Copy link
Collaborator

Choose a reason for hiding this comment

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

You never set the value of @base_indent. Maybe you intended a getter for @indent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's wrong indeed. base indent is current indent. Maybe we should also kill the setter. It doesn't make sense.

Comment on lines +39 to +37
# @param indent [Integer]
# the default indentation amount for {group} and {nest}.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Despite being longer, I would much rather go with default_indent. We can live with indent if you find it too long.

Makes for a simpler, more declarative API.
It's still there since project initialization.
@stackmystack stackmystack force-pushed the rich-api branch 2 times, most recently from 4a10fd0 to 39e72cc Compare January 7, 2025 08:27
@Amine-Mike Amine-Mike force-pushed the rich-api branch 2 times, most recently from 7fa5121 to 9fbc705 Compare January 7, 2025 12:35
@stackmystack stackmystack merged commit d5a8c04 into main Jan 10, 2025
2 checks passed
@stackmystack stackmystack deleted the rich-api branch January 10, 2025 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants