Resolve ambiguities
Let me know what you think. I took the liberty of rewriting the tests using ReTest, as it allows for much faster iteration in development.
Maybe besides tests that only test the perceived problem of potential method ambiguities, I should also include some tests that test the actual result of the operations.
Hmmm, why didn't CI run here?
I assumed CI was not run because I am somehow not approved to be the trigger. It made sense to me that CI would not be triggered by a PR from some random contributor.
Sorry for the slow response here. Yeah, it's weird though because usually it pops up a button to ask if I want to approve CI for a first-time contributor. But I don't see anyway to kick off a CI run here.
That said, here's my thoughts:
- It feels hard to review this PR because of all the unrelated changes; I can't tell which tests are new and covering the changes because the entire files moved; it would be much easier to back out all the test file moving while we work thru the ambiguity issue and then do a follow up PR that's just about moving tests around
- I think Aqua/JET have some ways to automatically test for ambiguities? I can't quite remember the details, but it could be nice to hook into something automated like that to try and avoid future issues.
I have no idea why you aren't given the option to run the CI actions. Maybe Github knows this PR has a scope issue. ;-)
- I realized after creating the PR that I made a mistake in adjusting the tests so fundamentally in a PR that also includes other changes. I will create a new PR including only the first commit of this one, then it's almost entirely a "move".
- I was using Aqua.jl, that's how I found them. I could create an automated test for the ambiguities too, but that will fail until this PR (#86) is merged.
Codecov Report
Patch coverage: 87.50% and project coverage change: +0.58 :tada:
Comparison is base (
bc8aba3) 94.81% compared to head (8ea444c) 95.40%.
:exclamation: Current head 8ea444c differs from pull request most recent head b6af311. Consider uploading reports for the commit b6af311 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## main #86 +/- ##
==========================================
+ Coverage 94.81% 95.40% +0.58%
==========================================
Files 5 4 -1
Lines 1041 1000 -41
==========================================
- Hits 987 954 -33
+ Misses 54 46 -8
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/chainedvector.jl | 96.66% <87.50%> (+0.02%) |
:arrow_up: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Ok, finally got CI to run; can you take a look at the test failures? 1.3 is probably just too old, so we can bump that up to 1.6.
I've made some new commits to expand code coverage.
Resolve the conflicts and I think this is ready to go?
I think I resolved the conflict. CI/CD should be run to confirm. (I should maybe have done a FF on my "main" rather than a merge commit from this main, but alas)
Bump - is there anything holding this up?