-
Notifications
You must be signed in to change notification settings - Fork 935
Make requirements, test requirements more explicit; add [test] pip extra
#665
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
base: master
Are you sure you want to change the base?
Conversation
|
@ryan-williams If you can address the comment in the PR, we can merge this in. |
|
just rebased and addressed the comment, gonna try to resuscitate a few of these, thanks for your patience |
84bb2ca to
fa91134
Compare
|
Just rebased this, not sure what this failure in "R tests on macos-latest" is about: |
|
I think the above was a spurious failure, fwiw; I tweaked this PR once more to alphabetize So, this should be ready for another look if anyone has time. |
|
thanks @savingoyal 🙏 looks like this is good to merge? |
romain-intel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Testing[44] @ 5da5e67 |
|
Testing[44] @ 5da5e67 |
|
Testing[44] had 3 FAILURES. |
|
Not sure how to parse "Testing[44] had 3 FAILURES." above. Where does that CI run? |
|
@ryan-williams : I am using your PR as a guinea pig for some tests we run internally. The CI is not accessible so don't worry about the message (it's a good comment though -- I will modify the message to indicate that the message can be ignored mostly). As you can see, it's not yet fully working (it worked fine when using branches from this repo but not so much from a fork (yet)). |
8f2be9a
8f2be9a to
1500528
Compare
(factored out of #612)