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.

Sign up for GitHub

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

Jump to bottom

Update to new API #169

Merged
merged 12 commits into from
Feb 25, 2020
Merged

Update to new API #169

merged 12 commits into from
Feb 25, 2020

Conversation

willtebbutt
Copy link
Member

See ChainRulesCore for related PR

nickrobinson251
nickrobinson251 requested changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

Tests pass on this branch if I add the corresponding branches

(ChainRules) pkg> add ChainRulesTestUtils#wct/frule-api-fix
(ChainRules) pkg> add ChainRulesCore#wct/frule-api-fix

so seems good to me, once the other PRs are merged and tagged and the compat here updated

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@willtebbutt
Copy link
Member Author

@nickrobinson251 let me know if you're happy for this to be merged now.

nickrobinson251
nickrobinson251 approved these changes Feb 21, 2020
View reviewed changes
Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

LGTM. Just need CI to pass before merge / your other PRs merged and tagged first.

Thanks for your work on this!

oxinabox
oxinabox reviewed Feb 21, 2020
View reviewed changes
.travis.yml Outdated
@@ -10,7 +10,6 @@ julia:
- nightly
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- nightly
- 1.4

oxinabox
oxinabox approved these changes Feb 21, 2020
View reviewed changes
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

It is worrying how few tests needed to be updated

@willtebbutt
Copy link
Member Author

It is worrying how few tests needed to be updated

Yeah. This is what caused this slip up with the API in the first place -- we simply didn't have the range of things implemented to catch the problem. I think that the right place to resolve this issue is the core though -- I've implemented extra tests to prevent API regression, but presumably there are other things we're not handling properly (kwargs, for example).

@willtebbutt
Copy link
Member Author

@oxinabox @nickrobinson251 anyone able to reproduce the errors locally? Tests are passing for me on 1.3.1. Have checked versions, and I appear to be using the same as CI is using...

@oxinabox
Copy link
Member

Oh is this: #149 ?

I think we had travis disabled on 1.3 for reasons because it was just broken even though it worked fine in 1.3 on everyones local machines

@willtebbutt
Copy link
Member Author

Ahh I see. Will re-disable and merge once tests pass.

@willtebbutt willtebbutt merged commit 71a9789 into master Feb 25, 2020
@oxinabox oxinabox deleted the wct/frule-api-fix branch August 17, 2020 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickrobinson251 nickrobinson251 nickrobinson251 approved these changes

@oxinabox oxinabox oxinabox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@willtebbutt @oxinabox @nickrobinson251