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

fix: sync up with nightly futures_api #176

Merged
merged 13 commits into from
Apr 18, 2019

Conversation

prasannavl
Copy link
Contributor

@prasannavl prasannavl commented Apr 16, 2019

Sync up the new changes in the nightly futures_api: futures dependency to 0.3.0-alpha.14
Also, updated all patch versions of other deps, while at it.

Blocker for successful build/release: http-rs/http-service#14

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

depends on changes to http-service, but looks good to me once those land!

Now that http-rs/http-service#14 has landed, update dep versions. (Though this also requires http-rs/http-service#17 now)
@prasannavl
Copy link
Contributor Author

prasannavl commented Apr 17, 2019

http-rs/http-service#14 has landed, but probably needs http-rs/http-service#17 too to land now.

Meanwhile, updated the http-service-* versions to correspond to the bumped versions.

Edit: Both landed. Ready to be merged.

@aturon
Copy link
Collaborator

aturon commented Apr 17, 2019

@prasannavl do you want to disable clippy on the travis config? We keep getting nightlies without it, which renders CI useless.

@prasannavl
Copy link
Contributor Author

prasannavl commented Apr 17, 2019

@aturon - ah, I did check for the nightly where everything built. Looks like I was off by 1 day. Fixed it up to later nightly. Hopefully, this should build (unless I was off by one day again).

Edit: Okay, I was indeed off by a day, but clippy built on this one - but let me fix up the lifetime lints as well, while at it. Give me a minute, checking it out locally to fix up

* expand on endpoint Fn's

* use ready! macro instead
@prasannavl
Copy link
Contributor Author

prasannavl commented Apr 17, 2019

Wait. What? This looks like bogus lint failures. (Although there are other linting pragmas that could be added and probably needs fixing like Debug impls for a bunch. But will open a separate PR for that.)

@aturon - or would you like me to add #[allow(clippy::needless_lifetimes)] to them?

Otherwise, I think this is ready to be merged.

@aturon
Copy link
Collaborator

aturon commented Apr 17, 2019

Weird error from Travis -- wanna take a look? Otherwise I can grab this locally and see if I can figure it out...

@prasannavl
Copy link
Contributor Author

I assumed it probably had to do with async await that clippy likely hadn't caught up yet. My first thought was to add #[allow(clippy::needless_lifetimes)], which should fix this -- unless there's something I'm missing.

Thought I'll do another PR with that and some more clippy pragmas next - but yes, would be helpful if you could just make sure that was a bogus lint error and I'm not missing something.

@Nemo157
Copy link
Contributor

Nemo157 commented Apr 18, 2019

@prasannavl I opened rust-lang/rust-clippy#3988 for it, it's definitely a clippy bug that it's warning about the lifetime generated as part of the async desugaring.

@yoshuawuyts
Copy link
Member

@prasannavl I'm +1 on adding #[allow(clippy::needless_lifetimes)] to squash the errors. If we then open a tracking issue that links to the clippy bug we can fix this once the clippy issue is resolved.

Add some good practise pragmas.

Additionally, this also worksaround clippy bug allowing it to pass with the addition of crate level `#![allow(clippy::needless_lifetimes)]`. It's applied on a crate level so as to avoid this from being added repeatedly across the modules or functions - and since it's strictly a  temporary measure. 

Related: http-rs#176
@prasannavl
Copy link
Contributor Author

prasannavl commented Apr 18, 2019

@yoshuawuyts - Done. So sorry, instead of a PR, I accidentally committed it directly on the master (forgetting I had commit rights on this now - since GitHub usually auto forks when I don't when making small changes directly)

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Apr 18, 2019

@prasannavl hah, yeah no worries. -- could you rebase this branch then tho so we can get this build to pass?

@prasannavl
Copy link
Contributor Author

yup, just doing that locally, and tests were still failing due to examples failing - so seeing if I can fix it quickly, otherwise will just push it rebased

@prasannavl
Copy link
Contributor Author

Looks like the recent PR #177 9e0a19a, was causing the failed doc tests after the futures api update.

All fixed, and we're building again!

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Excellent; thanks for doing this!

@yoshuawuyts yoshuawuyts merged commit 7e57a55 into http-rs:master Apr 18, 2019
@prasannavl prasannavl deleted the futures_update_2017_04 branch April 18, 2019 14:54
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.

5 participants