-
Notifications
You must be signed in to change notification settings - Fork 600
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
indent: handle consecutive linebreaks #2132
base: master
Are you sure you want to change the base?
Conversation
4f2d067
to
a63694c
Compare
lib/pry/indent.rb
Outdated
@@ -276,6 +280,14 @@ def in_string? | |||
!open_delimiters.empty? | |||
end | |||
|
|||
def already_indented?(output, prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these also have comments, like in_string?
, end_of_statement?
etc?
a63694c
to
b02d9be
Compare
@bcgraham you seem on the ball - wouldl you llke a commit bit? EDIT: given |
@banister Thanks, appreciate it 🙏. Not sure on etiquette or team culture around committing, so to err on the side of overclarifying:
|
spec/indent_spec.rb
Outdated
@@ -102,6 +102,36 @@ def number | |||
expect(@indent.indent(input)).to eq output | |||
end | |||
|
|||
it 'should properly indent nested code with consecutive linebreaks' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description doesn't reflect what we are trying to test here. Without context this test looks weird: we pass already indented code and verify that it's still indented. Nothing wrong with this kind of test per se but the description could be more clear.
- Avoid
should
, just sayit 'properly indents...'
- Since input is the same as output, I would make it a variable and reuse in the expectation
- I would change the description to say something like "given already indented code it returns self". That is, Indent didn't make any changes
Hope that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Avoid
should
, just sayit 'properly indents...'
👍. This was to be consistent with the surrounding descriptions, but I agree with avoiding should
in this manner. Changed to reflect this guidance.
- Since input is the same as output, I would make it a variable and reuse in the expectation
It's hard to see on GitHub, but the empty line on 111 becomes a line with two spaces on 124. Inserting hash signs (as specified in the comment at the top of the file) affects the test case, since the test case is about empty lines.
Given that context, what would you think about
input = [
'class C',
' def foo',
' :foo',
' end',
'',
' def bar',
' :bar',
' end',
'end'
].join("\n")
output = [
'class C',
' def foo',
' :foo',
' end',
' ',
' def bar',
' :bar',
' end',
'end'
].join("\n")
to prevent accidental editor whitespace linting, or something like
output = input.lines
output[4] = ' ' + output[4]
output = output.join
to express the diff between input
and output
? For now, I updated to the second one.
The above snippets are attempts to give you something concrete to comment on or perhaps say "yes" to, but ultimately, my preference here is "whatever @kyrylo & other maintainers prefer".
my_disposition.mp4
- I would change the description to say something like "given already indented code it returns self". That is, Indent didn't make any changes
This makes sense 👍; your example description for the submitted code is clearer than the original. I'm making this change. Let me know if, given your thoughts of the above, I should update the description (while keeping it clear!).
37cf767
to
22ef074
Compare
Not a fan of how GitHub manages PR discussion/changes, so I isolate changes resulting from PR feedback in their own commits, to be squashed prior to merging. |
22ef074
to
e6fcd2f
Compare
Took another gander at this and realized it could be simpler. I put together a version that returned the original input, e.g. expect(@indent.indent(input)).to eq input but found it actually made another test fail! # spec/commands/play_spec.rb, line 36
describe "playing a file" do
it 'should play a file' do
@t.process_command 'play spec/fixtures/whereami_helper.rb'
expect(@t.eval_string).to eq unindent(<<-STR)
# frozen_string_literal: true # <-- fixture has newline after this line, indenting removes
# rubocop:disable Layout/EmptyLineBetweenDefs
Let me know if you feel better about this change. |
Fixes #2094