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 SortedList method with function #3376

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

james-d-mitchell
Copy link
Contributor

@james-d-mitchell james-d-mitchell commented Mar 23, 2019

This PR adds a method for SortedList that takes a function as second argument.

Resolves #1002.

@coveralls
Copy link

coveralls commented Mar 23, 2019

Coverage Status

Coverage increased (+0.01%) to 85.152% when pulling 22bb293 on james-d-mitchell:sortedlist into 49bec58 on gap-system:master.

@wucas wucas added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Mar 24, 2019
@james-d-mitchell
Copy link
Contributor Author

I’m pretty sure that if the appveyor tests are re run this will pass, the failure looks like it happened because I force pushed almost immediately after opening the PR

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thanks, this is really helpful; but I think the documentation should be a tiny bit clearer.

lib/coll.gd Outdated Show resolved Hide resolved
gap> SortedList([2, 1, 3]);
[ 1, 2, 3 ]
gap> SortedList([2, 1, 3], {x, y} -> y < x);
[ 3, 2, 1 ]
Copy link
Member

Choose a reason for hiding this comment

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

Minor (completely optional) suggestion: Even better would be to also test that the input is not modified. E.g. by passing an immutable list to the function.

@codecov
Copy link

codecov bot commented Mar 24, 2019

Codecov Report

Merging #3376 into master will decrease coverage by <.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #3376      +/-   ##
==========================================
- Coverage   85.14%   85.13%   -0.01%     
==========================================
  Files         697      697              
  Lines      344069   344080      +11     
==========================================
+ Hits       292941   292946       +5     
- Misses      51128    51134       +6
Impacted Files Coverage Δ
lib/coll.gd 95.09% <100%> (+0.01%) ⬆️
lib/coll.gi 96.85% <88.88%> (-0.04%) ⬇️
src/sysmem.c 56.39% <0%> (-0.59%) ⬇️
src/gap.c 79.74% <0%> (-0.32%) ⬇️
src/stats.c 94.93% <0%> (-0.21%) ⬇️
src/streams.c 73.61% <0%> (-0.12%) ⬇️

@fingolfin
Copy link
Member

(and feel free to squash any suggested changes by me, I do not need "credit" for them in any form either ;-). And don't worry about AppVeyor failing, that is simply broken right now (but hopefully I can get it fixed via PR #3377)

@fingolfin
Copy link
Member

I took the liberty of adding Resolves #1002. to the issue description, so that this issue gets closed when we merge this PR.

Copy link
Contributor

@DominikBernhardt DominikBernhardt left a comment

Choose a reason for hiding this comment

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

This is my first review, so I might have missed something, but this looks good to me.

Co-Authored-By: Max Horn <max@quendi.de>
@fingolfin fingolfin merged commit 1eb79a6 into gap-system:master Mar 25, 2019
@markusbaumeister markusbaumeister added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 15, 2019
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: Add SortedList(list, cmpFunc)
8 participants