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 negative deltas on profiler #6106

Merged
merged 4 commits into from
Jan 4, 2018
Merged

Conversation

csabella
Copy link
Contributor

@csabella csabella commented Jan 1, 2018

Fixes #6032

@pep8speaks
Copy link

pep8speaks commented Jan 1, 2018

Hello @csabella! Thanks for updating the PR.

Line 589:80: E501 line too long (80 > 79 characters)

Comment last updated on January 04, 2018 at 17:39 Hours UTC

@csabella csabella changed the base branch from master to 3.x January 1, 2018 20:11
@csabella
Copy link
Contributor Author

csabella commented Jan 1, 2018

@ccordoba12 In the first commit, I fixed the issue without making many other changes. However, I noticed that some of the code was no longer needed because the formatting had been changed at some point, so in the second commit, I refactored to remove some of the unneeded code. If you don't want those changes, I can remove the second commit. Thanks!

@ccordoba12
Copy link
Member

ccordoba12 commented Jan 2, 2018

If you don't want those changes, I can remove the second commit. Thanks!

Don't worry, it's fine. But I'd like to see some tests for this feature, if it's not much too ask :-)

@ccordoba12 ccordoba12 added this to the v3.2.6 milestone Jan 2, 2018
@csabella
Copy link
Contributor Author

csabella commented Jan 3, 2018

@ccordoba12 I've added some tests, but just for the methods that I modified. I had trouble calling this program since it was in spyder_profiler and not spyder. I was able to run it (from within Spyder) to make sure the tests worked, but I didn't know how to get it to work from the command line.

Thanks!

I can also work on tests for the rest of the class, but I thought you might want to this in the new release, so I wanted to get it to you.

@ccordoba12
Copy link
Member

@csabella, I pushed one commit here to include the spyder_profiler directory among our tests.

@ccordoba12 ccordoba12 merged commit f5abe4f into spyder-ide:3.x Jan 4, 2018
ccordoba12 added a commit that referenced this pull request Jan 4, 2018
@csabella csabella deleted the issue6032 branch January 7, 2018 17:55
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