Resolve bug with sorting of table columns with mixed value types #1278
Conversation
| if type(left_data) is type(right_data): | ||
| return left_data < right_data | ||
| elif type(left_data) in (int, float) and type(right_data) is str and right_data == "": |
There was a problem hiding this comment.
I like to use isinstance() instead of type(). I believe type() is faster, but isinstance looks cleaner to me.
This could be rewritten as:
elif type(left_data) in (int, float) and isinstance(right_data, str) and not right_data
If right_data is a string, not right_data would be falsy. We can do the same on line 435.
This would just make the code look a bit cleaner in my opinion.
There was a problem hiding this comment.
To go a step further, I think it would be cleaner to try left_data < right_data directly. They don't need to have the same type to compare the two objects.
And isinstance is exactly the right answer here.
|
|
||
| If `left` and `right` are not the same type, we check if numerical and empty string are compared, if that is the | ||
| case, we assume empty string == 0. | ||
| Added this case for: https://github.com/LCA-ActivityBrowser/activity-browser/issues/1215""" | ||
| left_data = self.sourceModel().data(left, 'sorting') |
There was a problem hiding this comment.
We seem to have an inconsistent usage of single quotes versus double quotes. This is not a technical issue but consistency across the codebase will allow new people to easily come in and understand what's going on.
| class ABSortProxyModel(QSortFilterProxyModel): | ||
| """Reimplementation to allow for sorting on the actual data in cells instead of the visible data. | ||
|
|
||
| See this for context: https://github.com/LCA-ActivityBrowser/activity-browser/pull/1151 | ||
| """ | ||
| def lessThan(self, left, right): | ||
| def lessThan(self, left, right) -> bool: |
There was a problem hiding this comment.
What do you think about importing QModelIndex for the left and right arguments so that we can have better type hinting support?
Try this:
from PySide2.QtCore import QModelIndex```
Then update the function signature to this:
def lessThan(self, left: QModelIndex, right: QModelIndex) -> bool:```
If we try to be consistent with type hinting, then we will be able to detect errors by using linters like mypy.
|
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
Feel free to open a PR, I'm happy to merge it!
The reason this PR exists is because
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
Feel 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 :) |
resolve #1215
Checklist
bug,feature,ui,change,documentation,breaking,cias they show up in the changelog.