Skip to content

Conversation

@nsajko
Copy link
Member

@nsajko nsajko commented May 25, 2025

Fixes #81

@nsajko nsajko marked this pull request as draft May 25, 2025 09:29
@codecov
Copy link

codecov bot commented May 25, 2025

Codecov Report

Attention: Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.27%. Comparing base (751021f) to head (514d819).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/collect_as.jl 93.10% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
- Coverage   96.71%   94.27%   -2.45%     
==========================================
  Files           5        5              
  Lines         274      227      -47     
==========================================
- Hits          265      214      -51     
- Misses          9       13       +4     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nsajko nsajko force-pushed the excise_collect_as branch 3 times, most recently from 4f1a0f5 to 5a1002a Compare May 25, 2025 13:13
@nsajko nsajko changed the title depend on CollectAs.jl, excise collect_as depend on Collects.jl, excise collect_as May 29, 2025
@nsajko

This comment was marked as outdated.

@nsajko nsajko force-pushed the excise_collect_as branch 3 times, most recently from 5e362b8 to 25fa738 Compare June 3, 2025 12:42
@nsajko

This comment was marked as outdated.

@nsajko nsajko force-pushed the excise_collect_as branch from 25fa738 to 64b45eb Compare June 3, 2025 12:50
@giordano
Copy link
Collaborator

giordano commented Jun 3, 2025

The vector -> matrix conversion (or any other M -> N with N =! 1) would be ambiguous, I'm not entirely sure allowing only the X -> vector conversion is worthwhile (especially since Base's Vector doesn't allow that at the moment, although it could be changed)

@nsajko
Copy link
Member Author

nsajko commented Jun 3, 2025

OK, will make it throw. EDIT: done.

@nsajko nsajko force-pushed the excise_collect_as branch from 64b45eb to 8e8d46d Compare June 3, 2025 13:18
@nsajko nsajko force-pushed the excise_collect_as branch from 8e8d46d to a791cf2 Compare June 3, 2025 13:46
@nsajko nsajko force-pushed the excise_collect_as branch from a280f11 to 514d819 Compare June 3, 2025 14:00
@nsajko nsajko marked this pull request as ready for review June 3, 2025 14:04
end
function push!!(::Type{<:AbstractVector}, v::Vector, e)
push(v, e)
function infer_ndims_impl(::Base.IteratorSize)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an easy test to exercise this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already exercised (checked by replacing 1 with error("exercised!")). This is just this issue again:

@nsajko nsajko merged commit 1be0830 into JuliaArrays:main Jun 3, 2025
10 of 12 checks passed
@nsajko nsajko deleted the excise_collect_as branch June 3, 2025 16:41
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.

let a dependency package own collect_as?

2 participants