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

Adding command continue_always and continue_breakpoint #377

Merged
merged 10 commits into from
Jan 20, 2019

Conversation

renatocassino
Copy link
Contributor

Hello.

This PR is for this issue #366 .
I added two commands c[ont[inue]_]a[lways] and c[ont[inue]_]b[reak[point]] (I can change the names if you want). The first command ignore all next breakpoints and the second ignore all breakpoints in the same file and same line.

I started making a design pattern state in ContinueCommand, but the code started to become a few complex and I separated in two new commands.

In ContinueAlwaysCommand, the code change the always_run in list command to avoid print many things in console. If you prefer I can remove this one.

I wrote tests and all passed in my CI. https://travis-ci.org/tacnoman/byebug
If you want any refactor or change, ask to me and I will change.

Thanks for this.

@bak1an
Copy link
Contributor

bak1an commented Nov 23, 2017

Great stuff @tacnoman! Literally every time I leave byebug in a loop I wish it had something like that.

However, I would like to discuss the behavior of those commands a bit and maybe simplify them.

It looks like continue_breakpoint one can just be replaced by disabling a breakpoint or making it conditional in the first place. I guess we can just omit it to avoid introducing new commands that do not add really new features.

continue_always is more useful but looks a bit too aggressive to me. Does it completely disable byebug never to be called again? If yes - would it make sense to continue_always only if program stops again on that very same byebug call but stop on the other ones (so it behaves kind of like continue_breakpoint now)?

So I can run something like:

something.each do |x|
  # some code to debug
  byebug
end

# and this is the other loop in totally different place
otherthings.each do |y|
  # more code to debug here
  byebug
end

And when I am done with the first loop, I hit continue_always and get stopped in the second loop, skipping future stops in the first one.

Would that make sense to you?

cc @deivid-rodriguez

PS does it seem like we can invent a new name for continue_always, like run or something?

@deivid-rodriguez
Copy link
Owner

deivid-rodriguez commented Nov 24, 2017

Hi @tacnoman @bak1an, thanks for the PR and discussion! It's definitely a problem I continuously run into myself, and would love it fixed!

What do you think about copying gdb's behavior? It has two related commands:

  • c[ontinue] [<ignore-count>]: Resume program execution, at the address where your program last stopped; any breakpoints set at that address are bypassed. The optional argument ignore-count allows you to specify a further number of times to ignore a breakpoint at this location; its effect is like that of ignore (see Break Conditions).

  • advance <location> Continue running the program up to the given location.

This would involve making a backwards incompatible change on continue semantics and moving the old continue <line-number> behavior to a new command (advance?).

So when you're in a loop, you could say continue with a high enough ignore count, in order to get out of it.

Do you think this would solve the problem?

Ref: https://sourceware.org/gdb/current/onlinedocs/gdb/Continuing-and-Stepping.html#Continuing-and-Stepping.

@cyrilchampier
Copy link

continue X, as gdb describes it seems a good behavior indeed !

@deivid-rodriguez
Copy link
Owner

Hi @tacnoman. I decided to introduce continue! as an equivalent to your continue_always command since it doesn't add too much complexity to the current continue command. See #524.

But I think your continue_breakpoint command can be useful as is. The previous proposal of changing the meaning of the integer argument to continue to be an "ignore count" could be interesting but still doesn't completely solve the problem because you would have to guess the duration of loops while debugging them, and specifying a very high number as an alternative sounds worse for the user than a separate command.

Are you still interested in this? If yes, can you remove the continue_always command and rebase this PR?

@renatocassino
Copy link
Contributor Author

Hello @deivid-rodriguez

I'm interested yet.

I should make this today. If you prefer, I can remove the command continue_breakpoint and make another PR to the future.

The behavior in continue_breakpoint is:

  • save the file and line
  • ignore all breakpoints in this file/line
  • if arrive in another breakpoint, stop

With this behavior I don't need to know how many loops are sill missing

// arr = long array
arr.each do |value|
  byebug
  puts value
end

byebug

In line 2, if I continue_breakpoint (I didn't like this name too) will ignore all breakpoints in line 2 until arrive in line 7

I'll remove the continue_breakpoint, make a rebase and change the command continue_always to c[ont[inue]]!

@renatocassino
Copy link
Contributor Author

I changed the code, but I kept the commit with continue_breakpoint, because if in the future we decided to create this command, we can get the code in the commit list.

All tests passed

Command:

  • continue!
  • cont!
  • c!

I created another command file, but if you prefer i can merge with the command.rb, but I think that this command separated are better to avoid a complex regex to solve two different commands.

@deivid-rodriguez
Copy link
Owner

deivid-rodriguez commented Jan 3, 2019

@tacnoman I'm sorry I probably didn't explain my suggestion properly. I have already implemented continue! in #524, so I was asking for the opposite: remove that part from this PR, so we can introduce continue_breakpoint via this PR.

I have looked at how it works and I like it. I'm only not convinced by the current naming but I don't have a better suggestion so let's do it as it is.

Thanks and sorry for the confusion!

@renatocassino
Copy link
Contributor Author

Hello @deivid-rodriguez
I understood wrong, but no problem

I can change again to keep only the continue_breakpoint

I'll make this today at night

If I think a better name, I'll change

@deivid-rodriguez
Copy link
Owner

Thanks @tacnoman!!

@renatocassino
Copy link
Contributor Author

I made it
But there is a problem. The command ListCommand has the attribute always_runwith value 2

When skip the breakpoint the stdio are printing the lines of code many times

In command_always I was setted this attribute as 0, but I think that isn't a good idea change the attribute for another command, right?

Is there a way to avoid this?

The command are working

Another question. What's your opinion about the command called skip_breakpoint instead of continue_breakpoint?

@deivid-rodriguez

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.

Looking great, just added a few minor comments! Also, can you rebase and add a changelog entry?

lib/byebug/commands/continue.rb Outdated Show resolved Hide resolved
lib/byebug/commands/continue_breakpoint.rb Outdated Show resolved Hide resolved
lib/byebug/commands/continue_breakpoint.rb Outdated Show resolved Hide resolved
lib/byebug/commands/continue_breakpoint.rb Outdated Show resolved Hide resolved
test/commands/continue_breakpoint_test.rb Outdated Show resolved Hide resolved
@deivid-rodriguez
Copy link
Owner

But there is a problem. The command ListCommand has the attribute always_runwith value 2
When skip the breakpoint the stdio are printing the lines of code many times

Right. It's not pretty but the only obvious solution I can think of is to do what you did: removing list as an autocommand for the duration of continue_breakpoint. I'll try to think of how to make this better but for now I think we can use your solution?

@deivid-rodriguez
Copy link
Owner

Another question. What's your opinion about the command called skip_breakpoint instead of continue_breakpoint?

Yeah, I think it's better. The regular continue command also continues until a breakpoint, so I think skip_breakpoint is better 👍.

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.

Other than the nitpick about the test style, which I can live without, this is almost ready from my side. Only needs a changelog entry and a new row in the table of commands in the README file.

lib/byebug/commands/continue.rb Outdated Show resolved Hide resolved
@deivid-rodriguez
Copy link
Owner

I was thinking about maybe calling this new command just skip?

@renatocassino
Copy link
Contributor Author

Changed to skip

@renatocassino
Copy link
Contributor Author

@deivid-rodriguez I think that all is done now :D

The command is skip or sk. The command s is already the step
I updated the GUIDE.md too

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.

@tacnoman Thanks for following up. I gave the diff another look and added some tiny extra comments.

Also, could you add a new changelog entry to the "Unreleased > Added" section, and a new entry with the new command the to the table of commands in the README file?

lib/byebug/commands/skip.rb Outdated Show resolved Hide resolved
lib/byebug/commands/skip.rb Outdated Show resolved Hide resolved
lib/byebug/commands/skip.rb Outdated Show resolved Hide resolved
@deivid-rodriguez
Copy link
Owner

deivid-rodriguez commented Jan 20, 2019

Regarding the changelog entry, it could read

* [#377](https://github.com/deivid-rodriguez/byebug/pull/377): `skip` to continue until the next breakpoint as long as it is different from the current one. You can use this command to get out of loops, for example ([@tacnoman]).

@renatocassino
Copy link
Contributor Author

Will you add in CHANGELOG or I must add? @deivid-rodriguez

@deivid-rodriguez
Copy link
Owner

If you can add it, I appreciate it!

@renatocassino
Copy link
Contributor Author

Done :D

@deivid-rodriguez
Copy link
Owner

Great, thanks also for fixing my typo in your username! :)

@deivid-rodriguez
Copy link
Owner

Last thing missing is to add a new row for the new command to the README table. Could you do it?

@renatocassino
Copy link
Contributor Author

Added now

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 so much! I'll merge after CI!

@deivid-rodriguez deivid-rodriguez merged commit 2f03a94 into deivid-rodriguez:master Jan 20, 2019
@deivid-rodriguez
Copy link
Owner

deivid-rodriguez commented Jan 20, 2019

Appveyor is too slow, merged straight away!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 25, 2019
## [11.0.0] - 2019-02-15

### Added

* [#377](deivid-rodriguez/byebug#377): `skip` to continue until the next breakpoint as long as it is different from the current one. You can use this command to get out of loops, for example ([@tacnoman]).
* [#524](deivid-rodriguez/byebug#524): `continue!` (or `continue unconditionally`) to continue until the end of the program regardless of the currently enabled breakpoints ([@tacnoman]).

### Fixed

* [#527](deivid-rodriguez/byebug#527): `break` help text to clarify placeholders from literals.
* [#528](deivid-rodriguez/byebug#528): `quit!` help to not show a space between "quit" and "!".

### Removed

* Support for MRI 2.2. Byebug no longer installs on this platform.
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

4 participants