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: Add natural sort for columns #34

Closed
wants to merge 4 commits into from
Closed

PR: Add natural sort for columns #34

wants to merge 4 commits into from

Conversation

skjerns
Copy link
Contributor

@skjerns skjerns commented Jan 16, 2019

It sorted columns as strings, so 10.0 was before 1.0 (see #33 )
I've inserted some code snippet from https://stackoverflow.com/questions/21030719/sort-a-pyside-qtgui-qtreewidget-by-an-alpha-numeric-column to implement natural sort on the table elements.

Additionally I've fixed a bug that prevented the 'Profile by Line' button from working (caused by wdir being set to False and the call to self.process.setWorkingDirectory(wdir)). Excuse that I didn't create another PR for this!

@coveralls
Copy link

Coverage Status

Coverage increased (+1.6%) to 68.75% when pulling 14247cc on skjerns:patch-1 into 8052536 on spyder-ide:master.

@coveralls
Copy link

coveralls commented Jan 16, 2019

Coverage Status

Coverage increased (+2.0%) to 69.656% when pulling 9e39932 on skjerns:patch-1 into b5f5894 on spyder-ide:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.6%) to 68.762% when pulling 14247cc on skjerns:patch-1 into 8052536 on spyder-ide:master.

@ccordoba12 ccordoba12 changed the title Added natural sort for columns PR: Add natural sort for columns Jan 16, 2019
@ccordoba12
Copy link
Member

Thanks for your help with this one.

@jitseniesen, could you review this one? Thanks!

@jitseniesen
Copy link
Member

@jitseniesen, could you review this one? Thanks!

Sure.

@skjerns
Copy link
Contributor Author

skjerns commented Feb 15, 2019

@jitseniesen can we merge it?

@jitseniesen
Copy link
Member

@skjerns Sorry, I just found your PR again. Are you still interested in having it merged? If so, can you please explain the code for natural_sort_key, because I don't understand it?

@skjerns
Copy link
Contributor Author

skjerns commented Oct 11, 2019

this is some code snippet that I copied from elsewhere on stackoverflow. It is especially for widgets natural sorting.

in a general sense it splits up the lines into tuples and keeps all numbers it finds as floats. Then these tuples can be compared.

e.g.

'34.25%'

will be split up into float('34.25%') and '%', so when comparing (=sorting) them, the 34.25 will be compared by it's numerical value and not by it's string value.

There are certainly easier ways to do it for this usecase, but this is a generalized solution that will also work with other items.

@jitseniesen
Copy link
Member

Thanks, I get it now: this allows the same comparison function to be used for the text and numerical columns.

Could you please add the URL where you copied this from in a comment, and ideally add a doc string to TreeWidgetItem and/or natural_sort_function with some explanation?

Additionally I've fixed a bug that prevented the 'Profile by Line' button from working (caused by wdir being set to False and the call to self.process.setWorkingDirectory(wdir)). Excuse that I didn't create another PR for this!

Can you please explain what bug this is and how I can reproduce it? The button seems to work when I tried it and I don't understand the changes you made.

@skjerns
Copy link
Contributor Author

skjerns commented Oct 24, 2019

Thanks, I get it now: this allows the same comparison function to be used for the text and numerical columns.

Could you please add the URL where you copied this from in a comment, and ideally add a doc string to TreeWidgetItem and/or natural_sort_function with some explanation?

done!

Additionally I've fixed a bug that prevented the 'Profile by Line' button from working (caused by wdir being set to False and the call to self.process.setWorkingDirectory(wdir)). Excuse that I didn't create another PR for this!
Can you please explain what bug this is and how I can reproduce it? The button seems to work when I tried it and I don't understand the changes you made.

If you click the button once it works, but the second time it doesn't finish, this is fixed now

@skjerns skjerns closed this Oct 24, 2019
@skjerns skjerns reopened this Oct 24, 2019
@skjerns
Copy link
Contributor Author

skjerns commented Dec 12, 2019

... @jitseniesen can this be merged? one year later?

@jitseniesen
Copy link
Member

I am sorry for taking so long. I have been rather busy recently and I rarely if ever use the plugin myself.

I am afraid I cannot reproduce the bug in the current version from git. I did try clicking the "Profile by line" button multiple times. Looking at the code, it is not obvious to me how wdir can be set to False.

Can you explain more fully how to reproduce the bug using the current code, also mentioning what version of spyder and spyder-line-profiler you are using? Perhaps the bug is only present in the release version of spyder-line-profiler?

@skjerns
Copy link
Contributor Author

skjerns commented Dec 16, 2019

It's some time ago, so I'm not 100% sure, but I think the problem was that the QT-function that calls the start() doesn't initiate wdir with None, but calls the function with False, making the function fail. I've added that it treats False the same as None .

I'll test this again and report back, but I'm using the release version from pip indeed.
I've already upgraded to 4.0, so line-profiler isn't working anyway anymore.

@jitseniesen
Copy link
Member

Okay, I think I understand it now. Another user contributed commit 08b0a34 in 2018 which (in a different way) fixes the problem that you are describing. However, we have never made a release with this fix in it.

I will split the natural sort commits off and merge them, because they are a nice improvement. Then we will do a release with the 2018 fix, your natural sort, and the necessary changes to make the plugin compatible with Spyder 4.

@skjerns
Copy link
Contributor Author

skjerns commented Dec 16, 2019

great plan! thanks alot! was already wondering about the other changes in the file which weren't by me.

looking forward to using line-profiler in spyder 4.0 :)

@jitseniesen
Copy link
Member

As mentioned above, I split the sorting part of this PR in another PR, namely #38. The other part is no longer necessary.

Thanks @skjerns for your contribution and your patience.

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