activity-browser
activity-browser copied to clipboard
Resolve bug with sorting of table columns with mixed value types
resolve #1215
Checklist
- [x] Keep pull requests small so they can be easily reviewed.
- [ ] Update the documentation, please follow the numpy style guide.
- [ ] Update tests.
- [x] Categorize the PR by setting a good title and adding one of the labels:
bug,feature,ui,change,documentation,breaking,cias they show up in the changelog. - [x] Link this PR to related issues by using closing keywords.
- [x] Add a milestone to the PR (and related issues, if any) for the intended release.
- [x] Request a review from another developer.
coverage: 50.63% (-0.02%) from 50.652% when pulling e84b94fdb804504d528c050fb53fdd62a8ec5154 on marc-vdm:sort_real_values into 94ddba5471b38a0a57bc33a705f5092432fec5d5 on LCA-ActivityBrowser:master.
Thanks for your reviews both, that's is always welcome 👍! though more useful on PRs that are open ;)
I'm writing one reply for this PR to all your comments, that's a bit easier than writing a bunch of comments imo
@ughstudios I like to use isinstance() instead of type()
Feel free to open a PR, I'm happy to merge it!
@cmutel To go a step further, I think it would be cleaner to try
left_data < right_datadirectly
The reason this PR exists is because left_data < right_data didn't work, see linked issue. This is also documented in code (L426-L429)
@ughstudios We seem to have an inconsistent usage of single quotes versus double quotes
You are correct, our code isn't very consistent at the moment. This is something we're working on, see #1048 for general cleanup plans
@ughstudios What do you think about importing
QModelIndex
Feel free to open a PR, I'm happy to merge it!
Thanks for your reviews both, that's is always welcome 👍! though more useful on PRs that are open ;)
I'm writing one reply for this PR to all your comments, that's a bit easier than writing a bunch of comments imo
@ughstudios I like to use isinstance() instead of type()
Feel free to open a PR, I'm happy to merge it!
@cmutel To go a step further, I think it would be cleaner to try
left_data < right_datadirectlyThe reason this PR exists is because
left_data < right_datadidn't work, see linked issue. This is also documented in code (L426-L429)@ughstudios We seem to have an inconsistent usage of single quotes versus double quotes
You are correct, our code isn't very consistent at the moment. This is something we're working on, see #1048 for general cleanup plans
@ughstudios What do you think about importing
QModelIndexFeel free to open a PR, I'm happy to merge it!
It looks like I don't have the ability to make new branches? Can you give me access? I can make them locally of course, but in order to open a PR it needs to be on the github repo.
Happy to see you managed to figure it out. Feel free to have a read through CONTRIBUTING.md for such info like forking/branching in the future :)