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

Add optional parameter to sort nulls last #3474

Merged
merged 5 commits into from
Nov 7, 2018
Merged

Conversation

lbeaufort
Copy link
Member

@lbeaufort lbeaufort commented Nov 5, 2018

Summary (required)

Currently, nulls will sort last by default when sorting ascending - this feature adds an optional sort_nulls_last parameter which forces nulls to the bottom for descending sort. The default behavior of the API is unchanged. This is needed for proper display of front-end data tables.

How to test the changes locally

Impacted areas of the application

List general components of the application that this PR will affect:

Related PRs

fecgov/fec-cms#2488
fecgov/fec-cms#2491

@lbeaufort lbeaufort force-pushed the feature/sort-nulls-last branch from a9772fe to 7342696 Compare November 5, 2018 18:07
@codecov-io
Copy link

codecov-io commented Nov 5, 2018

Codecov Report

Merging #3474 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3474      +/-   ##
===========================================
+ Coverage    88.67%   88.67%   +<.01%     
===========================================
  Files           76       76              
  Lines         6323     6325       +2     
===========================================
+ Hits          5607     5609       +2     
  Misses         716      716
Impacted Files Coverage Δ
webservices/sorting.py 88.33% <100%> (+0.4%) ⬆️
webservices/utils.py 91.66% <100%> (ø) ⬆️
webservices/args.py 97.52% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79f02c0...6685559. Read the comment docs.

@lbeaufort lbeaufort changed the title [WIP] Add flag to sort nulls last Add optional flag to sort nulls last Nov 5, 2018
@lbeaufort lbeaufort changed the title Add optional flag to sort nulls last Add optional parameter to sort nulls last Nov 6, 2018
@fec-jli
Copy link
Contributor

fec-jli commented Nov 7, 2018

It works great.
one question. Can we set sort_nulls_last=True as default value? most of our cases want to sort_nulls_last=True? right? Thanks

@lbeaufort
Copy link
Member Author

Thanks for taking a look @fec-jli! While that’s my preference, I didn’t want to change the default behavior of the API - it’s arguably a breaking change to do so.

@lbeaufort
Copy link
Member Author

This CMS PR will default the front-end data tables to always sort nulls last:
fecgov/fec-cms#2491

Copy link
Contributor

@fec-jli fec-jli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works great. Thanks

@JonellaCulmer JonellaCulmer removed the request for review from pkfec November 7, 2018 18:42
Copy link
Contributor

@qqss88 qqss88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests look good.

I am a little confused on those 2 arguments:
'default_sort_nulls_last=False, sort_nulls_last=False'- it seems like 'sort_nulls_last' is not used in the function.

It's good to add some doc string to the function we modified.

@lbeaufort
Copy link
Member Author

@qqss88 that's a good catch, thanks! They were unused so I removed them with my latest commit. I should have time to add docstrings, but I want to make sure I get my other issue for this sprint addressed first.

@fec-jli fec-jli merged commit 2e542aa into develop Nov 7, 2018
@lbeaufort lbeaufort deleted the feature/sort-nulls-last branch November 8, 2018 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants