activity-browser icon indicating copy to clipboard operation
activity-browser copied to clipboard

Resolve bug with sorting of table columns with mixed value types

Open marc-vdm opened this issue 1 year ago • 1 comments

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, ci as 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.

marc-vdm avatar Apr 20 '24 13:04 marc-vdm

Coverage Status

coverage: 50.63% (-0.02%) from 50.652% when pulling e84b94fdb804504d528c050fb53fdd62a8ec5154 on marc-vdm:sort_real_values into 94ddba5471b38a0a57bc33a705f5092432fec5d5 on LCA-ActivityBrowser:master.

coveralls avatar Apr 20 '24 13:04 coveralls

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_data directly

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!

marc-vdm avatar Jun 03 '24 09:06 marc-vdm

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_data directly

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!

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.

ughstudios avatar Jun 03 '24 20:06 ughstudios

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 :)

marc-vdm avatar Jun 04 '24 08:06 marc-vdm