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

feat(server): return ListeningServer from Nickel::listen #334

Merged
merged 4 commits into from
Jul 9, 2016

Conversation

Ryman
Copy link
Member

@Ryman Ryman commented May 4, 2016

Related to #328.

The tests within this repo are a bit of a hack (require shelling out to subprocesses involving cargo), with this change we can instead make it easier to integration test a server (or multiple instances of one) all within the same process which should be easier and faster.

The code within the integration test example would get tested using a normal cargo test, but since it's in our example files I had to use a workaround to get it compiled directly into our integration test runner.

BREAKING CHANGE: This changes the return type of Nickel::listen, you can add an unwrap() to the call to maintain previous semantics.

@Ryman Ryman mentioned this pull request May 4, 2016
/// use nickel::Nickel;
///
/// let server = Nickel::new();
/// server.listen("127.0.0.1:6767");
/// let listening = server.listen("127.0.0.1:6767").expect("Failed to launch server");
/// println!("Listening on: {:?}", listening.socket());
Copy link
Member

Choose a reason for hiding this comment

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

Should this print be here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was to show the use of socket() to get the address, but I guess this example will print twice (since the options were not passed to disable printing). So I think it's fine, but I should make the intention more explicit perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I see. As you wish, I think we can just leave it like this.

@SimonTeixidor
Copy link
Member

Exciting! This looks nice to me, but I'm not known to do a lot of testing so let's see if we can get some user feedback.

@homu
Copy link
Contributor

homu commented May 9, 2016

☔ The latest upstream changes (presumably 2d91a0a) made this pull request unmergeable. Please resolve the merge conflicts.

@Ryman
Copy link
Member Author

Ryman commented May 18, 2016

Rebased on master

@homu
Copy link
Contributor

homu commented Jun 6, 2016

☔ The latest upstream changes (presumably f7658fa) made this pull request unmergeable. Please resolve the merge conflicts.

@euclio
Copy link
Contributor

euclio commented Jun 20, 2016

Is this still in flight?

…erver is listening on

BREAKING CHANGE: This changes the return type of Nickel::listen so it may break
some code. To fix broken code while maintaining the previous semantics, you can
add `unwrap` to the `listen` call.
@Ryman
Copy link
Member Author

Ryman commented Jun 23, 2016

@euclio just rebased. Do you have any suggested edits / concerns? If not I will try to merge this tomorrow.

@euclio
Copy link
Contributor

euclio commented Jun 23, 2016

Nope, looks good to me. Thanks for your work!

On Wed, Jun 22, 2016 at 10:25 PM Ryman notifications@github.com wrote:

@euclio https://github.com/euclio just rebased. Do you have any
suggested edits / concerns? If not I will try to merge this tomorrow.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#334 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABTxFlHnSNM4C80zvTthmIvnsqqHnRMPks5qOe6dgaJpZM4IXgqz
.

…ion tests

When this test is included in the integration suite, the import is no longer in
the root of the crate so the macro_use attribute is disallowed.
@euclio
Copy link
Contributor

euclio commented Jul 8, 2016

@Ryman friendly ping :)

@Ryman
Copy link
Member Author

Ryman commented Jul 9, 2016

@homu r+

@homu
Copy link
Contributor

homu commented Jul 9, 2016

📌 Commit c9abad7 has been approved by Ryman

homu pushed a commit that referenced this pull request Jul 9, 2016
…erver is listening on

BREAKING CHANGE: This changes the return type of Nickel::listen so it may break
some code. To fix broken code while maintaining the previous semantics, you can
add `unwrap` to the `listen` call.

Pull request: #334
Approved by: Ryman
homu pushed a commit that referenced this pull request Jul 9, 2016
homu pushed a commit that referenced this pull request Jul 9, 2016
homu pushed a commit that referenced this pull request Jul 9, 2016
…ion tests

When this test is included in the integration suite, the import is no longer in
the root of the crate so the macro_use attribute is disallowed.

Pull request: #334
Approved by: Ryman
@homu
Copy link
Contributor

homu commented Jul 9, 2016

⌛ Testing commit c9abad7 with merge ed54dbd...

@homu
Copy link
Contributor

homu commented Jul 9, 2016

☀️ Test successful - status

@homu homu merged commit c9abad7 into nickel-org:master Jul 9, 2016
KodrAus pushed a commit to KodrAus/nickel.rs that referenced this pull request Jul 23, 2016
…erver is listening on

BREAKING CHANGE: This changes the return type of Nickel::listen so it may break
some code. To fix broken code while maintaining the previous semantics, you can
add `unwrap` to the `listen` call.

Pull request: nickel-org#334
Approved by: Ryman
KodrAus pushed a commit to KodrAus/nickel.rs that referenced this pull request Jul 23, 2016
KodrAus pushed a commit to KodrAus/nickel.rs that referenced this pull request Jul 23, 2016
KodrAus pushed a commit to KodrAus/nickel.rs that referenced this pull request Jul 23, 2016
…ion tests

When this test is included in the integration suite, the import is no longer in
the root of the crate so the macro_use attribute is disallowed.

Pull request: nickel-org#334
Approved by: Ryman
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.

None yet

4 participants