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

Support more array mutation methods #29

Conversation

terasakisatoshi
Copy link
Contributor

This PR closes #21

@juliohm, @Vexatos

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ec93323) to head (51dcce5).
Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master       #29   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           50        57    +7     
=========================================
+ Hits            50        57    +7     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@terasakisatoshi
Copy link
Contributor Author

Should we still support 1.3?

@terasakisatoshi
Copy link
Contributor Author

Can I add Base.sizehint!(a::CircularVector, sz::Integer) = (sizehint!(parent(a), sz); a) method to pass CI for Julia nightly?

@terasakisatoshi
Copy link
Contributor Author

Oops...

 Got exception outside of a @test
  StackOverflowError:
  Stacktrace:
        [1] sizehint!(a::Vector{Int64}, sz::Int64; first::Bool, shrink::Bool)
          @ Base ./array.jl:1478
        [2] sizehint!
          @ ./array.jl:1478 [inlined]
        [3] sizehint!
          @ ~/work/CircularArrays.jl/CircularArrays.jl/src/CircularArrays.jl:136 [inlined]
        [4] sizehint!
          @ ./array.jl:1517 [inlined]
        [5] _append!
          @ ./array.jl:1317 [inlined]
        [6] append!
          @ ./array.jl:1309 [inlined]
        [7] push!(a::CircularVector{Int64, Vector{Int64}}, iter::Int64)
          @ Base ./array.jl:1310
        [8] _append!
          @ ./array.jl:1319 [inlined]
  --- the above 3 lines are repeated 79981 more times ---
   [239952] append!
          @ ./array.jl:1309 [inlined]
append!: Error During Test at /home/runner/work/CircularArrays.jl/CircularArrays.jl/test/runtests.jl:127
  Got exception outside of a @test
  StackOverflowError:

@juliohm
Copy link

juliohm commented Feb 6, 2024

@Vexatos can you please review this PR? It fixes the issue on recent Julia versions.

@Vexatos
Copy link
Owner

Vexatos commented Feb 6, 2024

The tests not passing for nightly and 1.3 would definitely have to be fixed before I review this. I also dislike the idea of dropping support for Julia 1.3 over this.

@terasakisatoshi
Copy link
Contributor Author

I also dislike the idea of dropping support for Julia 1.3 over this.

Could you explain why you want to pass CI for Julia v1.3? v1.6 is now LTS and v1.3 is obsolete.

@juliohm
Copy link

juliohm commented Apr 2, 2024

bump @Vexatos

@Vexatos
Copy link
Owner

Vexatos commented Apr 18, 2024

You make a good point. Since the next release will be a minor version up anyway, I can probably drop support for 1.3, a version which I was still using myself until a while ago. The large number of tests is greatly appreciated; I'll merge this once you add a test for sizehint! so that test coverage remains complete.

@Vexatos
Copy link
Owner

Vexatos commented Apr 18, 2024

I added the relevant test myself. It looks like the tests fail on Julia 1.6 right now, because the @test_throws do not produce the expected outcome as the syntax changed slightly.

@Vexatos Vexatos force-pushed the terasaki/support-more-array-mutation-methods branch from c464b31 to cfefe9a Compare April 18, 2024 14:18
@Vexatos
Copy link
Owner

Vexatos commented Apr 18, 2024

Looks like nightly tests are hitting an infinite loop involving push!. Perhaps this means that push! will have to be implemented manually.

@Vexatos Vexatos merged commit 2d1a408 into Vexatos:master Apr 18, 2024
11 checks passed
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.

Missing push! implementation
3 participants