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.

Sign up for GitHub

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

Jump to bottom

Unify #inspect output formatting in several places #5858

Merged
merged 1 commit into from Apr 28, 2018
Merged

Unify #inspect output formatting in several places #5858

merged 1 commit into from Apr 28, 2018

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Mar 23, 2018

Reference: #<Type …>
Struct: Type(…)

@Sija Sija changed the title Unify #inspect outut formatting in several places Unify #inspect output formatting in several places Mar 23, 2018
j8r
j8r reviewed Apr 3, 2018
View reviewed changes
src/time/location.cr Outdated
@@ -61,11 +61,11 @@ class Time::Location
end

def inspect(io : IO)
io << "Time::Zone<"
io << "Time::Zone("
io << offset
io << ", " << name
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are here, they can be merged to io << "Time::Zone" << offset << ", " << name

Copy link
Member

Choose a reason for hiding this comment

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

@j8r I don't think that would be an improvement.

src/time/location.cr Outdated
@@ -74,12 +74,12 @@ class Time::Location
getter? standard, utc

def inspect(io : IO)
io << "Time::ZoneTransition<"
io << "Time::ZoneTransition("
io << '#' << index << ", "
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

src/regex/match_data.cr Outdated
size.times do |i|
io << " "
io << " " if i > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the space string can be replaced by a char ' '

src/file/stat.cr Outdated
io << "#<File::Stat"
io << " dev=0x"
io << "File::Stat("
io << "dev=0x"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be merged to io << "File::Stat(dev=0x"

Copy link
Contributor Author

@Sija Sija Apr 11, 2018 •
edited

Choose a reason for hiding this comment

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

IMHO that just looks weird, especially since there are lines below having such format.

Copy link
Member

Choose a reason for hiding this comment

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

Why weird? Aesthetics? I don't see why these strings should not be combined.

j8r
j8r reviewed Apr 3, 2018
View reviewed changes
src/regex/match_data.cr Outdated
io << name_table.fetch(i, i) << ":" if i > 0
self[i]?.inspect(io)
end
io << ">"
io << ")"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, can be a char

src/regex/match_data.cr Outdated
size.times do |i|
io << " "
io << " " if i > 0
io << name_table.fetch(i, i) << ":" if i > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

":" => ':'

straight-shoota
straight-shoota reviewed Apr 11, 2018
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Just a few details to improve implementation, but otherwise looks good.

src/file/stat.cr Outdated
io << "#<File::Stat"
io << " dev=0x"
io << "File::Stat("
io << "dev=0x"
Copy link
Member

Choose a reason for hiding this comment

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

Why weird? Aesthetics? I don't see why these strings should not be combined.

src/regex/match_data.cr Outdated
size.times do |i|
io << ' '
io << ' ' if i > 0
Copy link
Member

Choose a reason for hiding this comment

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

Could be combined with the following line to avoid i > 0 twice.

src/regex/match_data.cr
size.times do |i|
pp.breakable
pp.breakable if i > 0
Copy link
Member

Choose a reason for hiding this comment

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

Put this line in the else branch to avoid comparing twice.

src/time/location.cr Outdated
@@ -61,11 +61,11 @@ class Time::Location
end

def inspect(io : IO)
io << "Time::Zone<"
io << "Time::Zone("
io << offset
io << ", " << name
Copy link
Member

Choose a reason for hiding this comment

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

@j8r I don't think that would be an improvement.

asterite
asterite approved these changes Apr 28, 2018
View reviewed changes
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Nice! But needs a rebase...

@Sija
Unify #inspect output formatting in several places
601bba0 
Reference#inspect: `#<Type …>`
Struct#inspect: `Type(…)`
@Sija
Copy link
Contributor Author

Sija commented Apr 28, 2018

@asterite rebased.

@asterite asterite requested a review from RX14 April 28, 2018 15:13
@RX14
Copy link
Contributor

RX14 commented Apr 28, 2018

Is it really only regex match data? cool

@RX14 RX14 merged commit 9ba1ee8 into crystal-lang:master Apr 28, 2018
@RX14 RX14 added this to the Next milestone Apr 28, 2018
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018
@Sija @chris-huxtable
Unify #inspect output formatting in several places (crystal-lang#5858)
9cbe847 
Reference#inspect: `#<Type …>`
Struct#inspect: `Type(…)`
@makenowjust makenowjust mentioned this pull request Apr 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota left review comments

@j8r j8r j8r left review comments

@asterite asterite asterite approved these changes

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
kind:refactor topic:stdlib
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Sija @RX14 @asterite @straight-shoota @j8r