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

Precursor: Tide core isolation revamp #219

Merged
merged 1 commit into from
May 15, 2019
Merged

Precursor: Tide core isolation revamp #219

merged 1 commit into from
May 15, 2019

Conversation

prasannavl
Copy link
Contributor

@prasannavl prasannavl commented May 13, 2019 •
edited
Loading

Description

What's done

  • Move the main crate into tide and setup workspaces
  • Moves the examples into it's own

The above is still a non-breaking change and as such can be merged without really isolating the core and as such is meant to be a precursor PR to the actual isolation.

What's explicitly excluded from as a part of this PR

  • Move each middleware into it's own crate from the tide crate

Motivation and Context

#162

How Has This Been Tested?

All tests passed

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring and organisational changes

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@prasannavl prasannavl mentioned this pull request May 13, 2019
Merged
8 tasks
@prasannavl prasannavl force-pushed the 2019_05_tide_isolate_core branch from 56f75d1 to 512946b Compare May 13, 2019 16:08
@bIgBV bIgBV added this to the Sprint 2 milestone May 13, 2019
@prasannavl prasannavl requested review from Nemo157 and tirr-c and removed request for Nemo157 May 14, 2019 01:14
@prasannavl prasannavl force-pushed the 2019_05_tide_isolate_core branch from 7f378b6 to 36f5954 Compare May 14, 2019 01:51
tirr-c
tirr-c reviewed May 14, 2019
View reviewed changes
Copy link
Collaborator

@tirr-c tirr-c left a comment

Choose a reason for hiding this comment

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

It might be better to have each example as separate bin (under examples/src/bin/), so that it can be easily run. How do you think?

@prasannavl
Copy link
Contributor Author

prasannavl commented May 14, 2019 •
edited
Loading

@tirr-c - Ah, sounds good. I like it! However, that could be a separate PR? I don't think we need to mix it up with this.

I'll open out an issue for that separately?

tirr-c
tirr-c approved these changes May 14, 2019
View reviewed changes
Copy link
Collaborator

@tirr-c tirr-c left a comment

Choose a reason for hiding this comment

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

Sure thing!

@fairingrey fairingrey mentioned this pull request May 14, 2019
Merged
8 tasks
Nemo157
Nemo157 reviewed May 14, 2019
View reviewed changes
examples/Cargo.toml Outdated Show resolved Hide resolved
Nemo157
Nemo157 approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

LGTM

@wafflespeanut
Copy link
Contributor

Does it make sense to move the examples to tide crate and enable autoexamples = true in Cargo.toml instead of maintaining a separate crate for it? (I did that in my attempt here).

@prasannavl
Copy link
Contributor Author

prasannavl commented May 14, 2019 •
edited
Loading

@wafflespeanut - I think that would make sense for direct library based examples usually. Since we're going to have examples that span across middlewares and other deps, and potentially as independently runnable binary apps, I think it'd be easier to maintain a separate crate, which will also double up as an integration test.

Running should be easy once the bin splits happen.

@wafflespeanut
Copy link
Contributor

Okay cool. Is this good to go? Shall I branch from this and start working on splitting stuff?

@prasannavl
Copy link
Contributor Author

prasannavl commented May 14, 2019 •
edited
Loading

@wafflespeanut - I think so. But I'd probably prefer to sit on it for a couple of hours before merging just to let the clock run to make sure there's a sufficient window for folks to catch up.

Changes, if any should be minimal, so it's a good idea to base other work off this.
PS. Let me do a quick squash though.

@prasannavl prasannavl force-pushed the 2019_05_tide_isolate_core branch from 70dd65d to 85af357 Compare May 14, 2019 09:39
@wafflespeanut wafflespeanut mentioned this pull request May 14, 2019
Merged
9 tasks
@secretfader
Copy link

LGTM. That said, there's little reason to block other PRs until this one is completed. I elaborate more on that in #218.

Other PRs that were ready to go before this one was introduced should be merged.

@prasannavl
Copy link
Contributor Author

prasannavl commented May 14, 2019 •
edited
Loading

@secretfader - This is not at all cool. I really wish you didn't just go and merge your PR esp, without any one else weighing in on it in such a hurry esp, as I had explicitly expressed my concerns. #218 (comment) . Now all of the work on top, including ones which are ready to be merged will have to be rebased, even though I'm just waiting to let the clock run.

What really disconcerts me is that this is for a simple rename that will be extracted out into it's own crate any way shortly, and I really don't understand the hurry in that, and I've been trying to do the whole revamp in a non-breaking way as well as much as possible.

If you really must, I wish you had atleast discussed it, or asked other maintainers' suggestion.

And your supposed elaboration doesn't really help in your justification - This is not a first come first serve game we're playing - the idea is to keep the process efficient being respectful of each other's work. This could have easily waited.

If you need, please read discussion on discord for the merging of #194 - that's a much much larger work than what you just committed and we're keeping conduct respectful of other's work and trying to find the best middle path that's agreeable. What you're doing is not helpful.

@fairingrey fairingrey mentioned this pull request May 14, 2019
Merged
8 tasks
@prasannavl
isolate core revamp
d67ec8f 
fix cargo fmt

loosen up clippy to warnings on examples

snipe dev deps from core

eliminate some clippy warnings for examples

add publish false, and remove publish requirements

rename tide-examples to just examples
@prasannavl prasannavl force-pushed the 2019_05_tide_isolate_core branch from 85af357 to d67ec8f Compare May 15, 2019 01:24
@prasannavl prasannavl mentioned this pull request May 15, 2019
Open
@prasannavl
Copy link
Contributor Author

Alright, we're good to go! Since this will unblock other work to move forward, going ahead and merging this! /cc @wafflespeanut @fairingrey

@prasannavl prasannavl merged commit a50668f into http-rs:master May 15, 2019
This was referenced May 15, 2019
Closed
Closed
@prasannavl prasannavl deleted the 2019_05_tide_isolate_core branch May 15, 2019 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nemo157 Nemo157 Nemo157 approved these changes

@tirr-c tirr-c tirr-c approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Sprint 2
Development

Successfully merging this pull request may close these issues.
6 participants
@prasannavl @wafflespeanut @secretfader @Nemo157 @tirr-c @bIgBV