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

PR: Fix syncing of line number and plugin list for breakpoints and add unit tests. #5575

Merged
merged 5 commits into from
Oct 27, 2017

Conversation

csabella
Copy link
Contributor

@csabella csabella commented Oct 26, 2017

Fixes #2179

  1. The unit tests were written before the change to make sure the change fixed the bug reported.
  2. I refactored the code slightly to keep my change cleaner. I hope that's OK.
  3. With the unit tests, in the clear_breakpoints method, I put a note regarding the del. It's not calling the __del__ in the class.

@ccordoba12
Copy link
Member

Thanks @csabella for your contribution! As I asked you last time: is this an issue in our 3.x branch too? If so, please move your branch to 3.x.

@rlaverde, please review @csabella's work, given that you're also working with the debugger ;-)

@csabella
Copy link
Contributor Author

@ccordoba12 Technically it is a bug in 3.x, but since it's from an older bug report, I didn't know if you wanted to treat it as an enhancement instead. I'll be more careful to use the right branch in the future.

@ccordoba12 ccordoba12 added this to the v3.2.5 milestone Oct 26, 2017
@csabella csabella changed the base branch from master to 3.x October 26, 2017 14:12
@ccordoba12
Copy link
Member

@csabella, if an issue is marked as a bug and is present in 3.x, please solve it in 3.x. Only if it's marked as an enhancement, it needs to be worked in master.

I'll be more careful to use the right branch in the future.

Don't worry about it. In any case, you can ask us in the corresponding issue if it needs to be solved in master or 3.x.

@csabella
Copy link
Contributor Author

@ccordoba12 I had to make some changes to the tests for 3.x. I commented each line that I changed. Would it be possible to modify those lines before merging with master? The original version of the tests were passing under 4.0. Thanks!

Copy link
Member

@rlaverde rlaverde left a comment

Choose a reason for hiding this comment

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

LGTM

I understand the refactoring, It removes some unnecessary code

Thanks for all the tests

@rlaverde
Copy link
Member

I had to make some changes to the tests for 3.x. I commented each line that I changed.

Maybe something like this, could do the trick:

from spyder import version_info

if version_info > (4,):
    ...

Although I'm not sure if it's the best way to do it

@rlaverde
Copy link
Member

Btw, I'm not able to reproduce #2179, @csabella What was the error?

@ccordoba12
Copy link
Member

Maybe something like this, could do the trick

I also think we should follow a similar path regarding different behaviors for Spyder 3 and master in the tests you added @csabella.

@csabella
Copy link
Contributor Author

Btw, I'm not able to reproduce #2179, @csabella What was the error?

The error was that the red dot was being marked on a line indicating that it was a breakpoint, but the breakpoint wasn't listed in the breakpoint plugin.

To recreate:

  1. Add some lines of code to an editor.
  2. Double click or F12 a line to add a breakpoint. At this point, there's a red dot and it's in the list.
  3. Double click again to remove the breakpoint. It's gone from both.
  4. Now, on the same line, Shift-F12 or Shift-Double-Click for the edit condition dialog.
  5. Cancel from this dialog.
  6. The red dot remains, but the breakpoint isn't in the list on the plugin.

@csabella
Copy link
Contributor Author

I can't figure out the Travis error. The only change I made between this commit and the previous commit was to add a check the Spyder version number on lines that had been commented out. I would think that would make them basically the same, but yet, the last commit passed the tests and this one doesn't.

@ccordoba12
Copy link
Member

@csabella, I restarted the failing Travis slot. Sometimes we got segfaults in Travis that require a restart. But now that you're a core dev, you can restart the failing slots yourself :-)

@ccordoba12 ccordoba12 merged commit ed53e29 into spyder-ide:3.x Oct 27, 2017
ccordoba12 added a commit that referenced this pull request Oct 27, 2017
@csabella csabella deleted the issue2179 branch October 27, 2017 16:38
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