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 the byebug line # to make the output clickable #553

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

senhalil
Copy link

Adding the exact line number of the byebug statement to the end of the filename on the output makes the statement clickable in IDE's and editors supporting this functionality. Currently clicking the path goes to the top of the file. With the modification, clicking takes the user to the exact byebug line in the document.

For example in vscode it looks like follows:
vscode_byebug

Copy link
Owner

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks @senhalil, I like this 👍.

Can you rebase the PR, fix the tests and add a changelog entry under the "### Added" section explaining why we're doing this?

Adding the exact line number of the byebug statement to the end of
the filename on the output makes the statement clickable in IDE's
and editors supporting this functionality. Currently clicking the
path goes to the top of the file. With the modification, clicking
takes the user to the exact byebug line in the file.
@senhalil
Copy link
Author

I realised fixing the tests is not as straight forward as I thought at the beginning.

The complication comes from the fact that one can use list as many times as they want without modifying the frame.line.

Then the output becomes as follows:

 "[7, 16] in /tmp/byebug_example.rb:13",
 "7:       \"%1\"",
 "8:     end",
 "9:   end",
 "10:",
 "11:   byebug",
 "12:",
 "=> 13:   str = ByebugExampleClass.new.build_percentage_string",
 "14:",
 "15:   str",
 "16: end",
 "(byebug)",
 "",
 "[6, 8] in /tmp/byebug_example.rb:13",  <--------- THIS LINE
 "6:     def build_percentage_string",
 "7:       \"%1\"",
 "8:     end",
 "(byebug)"

13 is still useful for terminal debug sessions since it tells the next line to be executed; however, then the tests needs to be modified as such:

check_output_includes "[6, 8] in #{example_path}:13"

I don't know if you are okay with this modification.

To be honest, if someone is using list this means they are probably on terminal which means they don't need the "hyperlink' support; but I think giving the frame.line info still useful. What do you think?

@deivid-rodriguez
Copy link
Owner

Hi @senhalil! So... It sounds that just recalculating the middle line every time like you did in the beginning is a better option after all?

@senhalil
Copy link
Author

senhalil commented Jun 5, 2019

Hi @senhalil! So... It sounds that just recalculating the middle line every time like you did in the beginning is a better option after all?

Hi @deivid-rodriguez In my opinion, your suggestion is still the superior option, because it gives a useful info -- i.e., next instruction to be executed --, my original way didn't give any info (except average of two numbers :) ).

If someone is using list methods, the location of the instruction arrow might be useful to them.

So I suggest keeping the current version if you are not oppose to it.

PS: Is it me who needs to mark "Changes requested" as resolved or you?

@deivid-rodriguez
Copy link
Owner

Yeah, right.

I'm still worried that this might confuse some users into thinking this is a bug and we might get reports about it. Is there any way we can clarify the status line in this regard? Maybe something like:

Stopped at /path/to/file:<line>. Showing lines [min, max]

Or something like that?

@deivid-rodriguez
Copy link
Owner

PS: Is it me who needs to mark "Changes requested" as resolved or you?

I think it's me 👍.

@senhalil
Copy link
Author

senhalil commented Jun 5, 2019

I'm still worried that this might confuse some users into thinking this is a bug and we might get reports about it. Is there any way we can clarify the status line in this regard? Maybe something like:

I understand.

Stopped at /path/to/file:. Showing lines [min, max]

Or something like that?

I am happy with that wording, if you are. It is clean and clear.

Will make the necessary modif and commit this we

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.

None yet

2 participants