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

Added alternative names for fn describe and fn it #27

Merged
merged 1 commit into from
Dec 18, 2016

Conversation

regexident
Copy link
Collaborator

@regexident regexident commented Nov 14, 2016

Attempts to fix issues #5 and #6.

I also added the popular given -> when -> then variants while I was at it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 97.447% when pulling 6127ae8 on regexident:given-when-then into 7e8dc7c on mackwic:master.

@coveralls
Copy link

coveralls commented Nov 14, 2016

Coverage Status

Coverage increased (+0.1%) to 97.493% when pulling 61e6551 on regexident:given-when-then into 7e8dc7c on mackwic:master.

Copy link
Member

@mackwic mackwic left a comment

Choose a reason for hiding this comment

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

I like it ! :)

Some remarks:

  • please add some more tests
  • please add an example in the examples/ directory
  • please add an example in the header of src/context.rs file which highlight some of the aliases
  • add yourself to the contributors 😉

@@ -111,6 +111,46 @@ impl<'a> Context<'a> {
self.tests.push(Testable::Describe(String::from(name), child))
}

/// Interchangeable alternative name for [`describe`](struct.Context.html#method.describe).
///
/// See [`describe`](struct.Context.html#method.describe) for more info.
Copy link
Member

Choose a reason for hiding this comment

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

the links ! 👍

#[test]
fn it_can_call_when_inside_describe_body() {
describe("A Root", |ctx| ctx.when("nested describe", |_ctx| {}));
}
Copy link
Member

Choose a reason for hiding this comment

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

missing tests for context and specify. You may want a macro, like:

macro_rules! test_describe_alias {
  ($alias: ident) => {
    #[test]
    fn describe_has_alias_$alias() {
      describe("A Root", |ctx| ctx.$alias("nested describe", |_ctx| {}));
    }
  }
}

(made from memory, without testing, it may not compile...)

ctx.then("nested then", || Ok(()) as Result<(),()>)
})
})
});
Copy link
Member

Choose a reason for hiding this comment

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

this is nice indeed !

@mackwic
Copy link
Member

mackwic commented Nov 16, 2016

Thanks a lot !

If the PR is successfully merged, my policy is to make you a collaborator of the repository and give you the commit rights.

@regexident
Copy link
Collaborator Author

regexident commented Dec 17, 2016

Sorry for the delay, Thomas. 😢


concat_idents!(…, …)unfortunately doesn't fork work generating function names, so I had to go the slightly more verbose path.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 97.568% when pulling 20a3ce1 on regexident:given-when-then into 7e8dc7c on mackwic:master.

Copy link
Member

@mackwic mackwic left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, Thomas. 😢

No problem, we all have lives out there. OSS has the benefit to let us craft the code without a hurry. 😉


This is a GO for me, there is adequate documentation, examples and tests so that we'll know people will be able to use them. 👏

🎉 congrats @regexident ! 🎉 You are now a committer of if this repository !

The rules are very simple, you already know them:

  • don't commit directly to master, use features branches with PR
  • all code must be tested, public apis must be documented, examples should be provided when relevant
  • you can merge a PR after 10 days without comments on my part. You gained my trust after all
  • if someone merged a PR with code, he becomes a committer
  • be kind to people

... this should be in a CONTRIBUTING.md

@@ -19,13 +19,37 @@
//! ctx.it("use a `ctx` object", || Ok(()) as Result<(),()>)
//! });
//!
//! describe("Context::specify", |ctx| {
//! ctx.specify("can used as a drop-in alternative to `Context::describe`", |ctx| {
Copy link
Member

Choose a reason for hiding this comment

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

should be “can be used”, isn't it ?

{

let mut child = Context::default();
body(&mut child);
self.tests.push(Testable::Describe(String::from(name), child))
self.tests.push(Testable::Describe(name.into(), child))
Copy link
Member

Choose a reason for hiding this comment

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

great ! 👍 this is far better

@mackwic mackwic merged commit 23644cd into rust-rspec:master Dec 18, 2016
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

3 participants