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

delete! for PriorityQueue #344

Closed
wants to merge 4 commits into from
Closed

Conversation

scls19fr
Copy link
Member

Closes #343

@oxinabox
Copy link
Member

CI is being bad, but this LGTM.
I can merge this if you are happy with it
(and we changed the settings so we don't need CI to pass first)

@kmsquire
Copy link
Member

@oxinabox, it's better if we let CI pass first. While they're not perfect, they're there for a reason--to make sure as much as possible works! If something is broken about CI, it should ideally be fixed, not ignored.

In some cases, of course, waiting for CI to pass or fixing problems is not reasonable or possible for any of us (e.g., the latest Julia Windows builds haven't been updated, so the Julia 0.7 Appveyor tests are failing).

Anyway, I just restarted the failing job on Travis, which seems to have been a timeout. If that succeeds, it also might fix the coverage issues.

@oxinabox
Copy link
Member

I agree, and was likewise waiting. (I thought I kicked Travis to retry)
The comment was mores an an aside.

We should actually be passing on AppVeyor even with the old 0.7 builds as the version conditional I added to accumulators (which was what was broken) is linked to the exact build number.
THough it seemed to me recent AppVeyor failures have been downloading julia binary issues.
Which were in particular the CI tests I was thinking of when I mentioned bypassing them.

@scls19fr
Copy link
Member Author

scls19fr commented Nov 17, 2017

Personally, I won't have fixed doctest now.
I will first tackle Documenter.jl #346 / doctests #345 issue and after tackle this one

@codecov
Copy link

codecov bot commented Jul 29, 2018

Codecov Report

Merging #344 into master will decrease coverage by 2.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #344      +/-   ##
=========================================
- Coverage   98.65%   96.5%   -2.16%     
=========================================
  Files          30      31       +1     
  Lines        1716    2286     +570     
=========================================
+ Hits         1693    2206     +513     
- Misses         23      80      +57
Impacted Files Coverage Δ
src/priorityqueue.jl 99.14% <100%> (+0.18%) ⬆️
src/delegate.jl 40% <0%> (-60%) ⬇️
src/classified_collections.jl 85.71% <0%> (-14.29%) ⬇️
src/heaps/mutable_binary_heap.jl 92.79% <0%> (-7.21%) ⬇️
src/sorted_set.jl 93.71% <0%> (-6.29%) ⬇️
src/sorted_multi_dict.jl 92.92% <0%> (-6.01%) ⬇️
src/ordered_dict.jl 86.8% <0%> (-5.32%) ⬇️
src/accumulator.jl 95% <0%> (-5%) ⬇️
src/default_dict.jl 97.22% <0%> (-2.78%) ⬇️
src/ordered_set.jl 98.18% <0%> (-1.82%) ⬇️
... and 21 more

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 d6ee8e0...4d2834b. Read the comment docs.

Copy link
Member

@kmsquire kmsquire left a comment

Choose a reason for hiding this comment

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

@scls19fr, unfortunately, this PR needs to be updated for changes in DataStructures. Can you make those changes?

Delete the mapping for the given key in a priority queue, and return the priority queue.
# Examples
```jldoctest
julia> q = PriorityQueue(["a","b","c"],[2,3,1],Base.Order.Forward)
Copy link
Member

Choose a reason for hiding this comment

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

PriorityQueues no longer accept two iterables during construction (matching the corresponding change in other data structures like Dicts). Some alternatives include

q = PriorityQueue(Base.Order.Forward, "a"=>2, "b"=>3, "c"=>1)
q = PriorityQueue(Base.Order.Forward, zip(["a","b","c"],[2,3,1]))
q = PriorityQueue(zip(["a","b","c"],[2,3,1]), Base.Order.Forward)
q = PriorityQueue(Base.Order.Forward, [("a", 2), ("b", 3), ("c", 1)])

(The same change needs to be made in the docstring for the struct, if you're up for it.)

@@ -176,6 +176,12 @@ end
@test dequeue_pair!(pq) == ('e'=> 1)
@test dequeue_pair!(pq, 'b') == ('b'=>4)
@test length(pq) == 3

# delete!
pq = PriorityQueue(["a","b","c"],[2,3,1],Base.Order.Forward)
Copy link
Member

Choose a reason for hiding this comment

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

Same change needed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's sad to see that JuliaDocs/Documenter.jl#527 is not fixed!
We should have been able to catch this kind of error!

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe some people at JuliaCon hackaton should try to fix thiss first because that's very annoying not to be able to rely on doctests!

@scls19fr
Copy link
Member Author

Closed in favor of #428

@scls19fr scls19fr closed this Aug 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants