-
Notifications
You must be signed in to change notification settings - Fork 22
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
Set Yadda options #72
Conversation
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
644f302
to
0668fe9
Compare
Rebased. Looks like my editor did a bunch of white space changes in README.md, 😞 hiding the real change at the bottom. I bumped the version to 0.4.0 in anticipation of publishing soon. 🎉 How does setting the language by passing options to the featureParser work with setting English on the library in steps.js? This needs a bit of investigation before merging. ❓ |
Yeah, good catch! I think I didn't modify anything related to the localization feature compared to the previous blueprints. And I don't know off-hand how this should look like, but indeed this seems to need to be fixed! |
The language set for featureParser is used to parse the .feature files. The language set for the library is used to parse the *-steps.js files. Most likely these should match. With this latest change, the language may be configured in ember-cli-build.js and then is used for both the featureParser and the *-steps.js files. At build time, the configured language is:
I have manually tested the qunit side, but not mocha. Also need to consider how to add any automated tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
if (setupFn && (featureAnnotations.application || featureAnnotations.rendering || featureAnnotations.context)) { | ||
throw new Error('You must not assign any @application, @rendering or @context annotations to a scenario as well as its feature!'); | ||
if (setupFn && (featureAnnotations.setupapplicationtest || featureAnnotations.setuprenderingtest || featureAnnotations.setuptest)) { | ||
throw new Error('You must not assign any @setupapplicationtest, @setuprenderingtest or @setuptest annotations to a scenario as well as its feature!'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, how did I miss that? Good catch!
Absolutely! As my app was using Mocha, I tested the changes I did recently using an npm link, but in general this is certainly bad that were kind of blind on one eye. 🤔 I created #73 to track this... |
@albertjan @simonihmig Ready to merge and publish 0.4.0? |
Yes, go for it! 🎉 |
Nice one guys! Does one of you want to do the release tweet? Scream and shout about what we've done 😛? |
0.4.0 is published. Edit release notes as you see fit. Thanks for your work on this! One of you please do the tweeting. |
@sfbwalder I changed your release description a bit. It don't think that this does not work with Ember 2.x anymore! AFAIK it is not related to any Ember version at all. For example I maintain In the case of Mocha I think you could even rewrite your annotations to use Mocha's old setup functions, and use this addon this way with the old testing setup. In case of QUnit, that's maybe also somehow possible, but not as easy as we cannot use the custom Just for clarification ;) |
The privilege belongs to the maintainer I think!? 😉 |
😳 😫 😬 duh, thanks for the fix. |
haha, you guys did all the work! I'll retweet! 😄 |
This replaces #42, and is intended to be rolled into 0.4.0.
It allows options for Yadda to be passed through from
ember-cli-build.js
:The language string is converted to a yadda localisation module in ember-cli-yadda's feature parser b/c it was an easy place to insert it (yadda is available here). It could probably go somewhere is if this doesn't seem right.
The location of the Yadda Configuration section in the readme file will need to be changed when #71 is merged, so this is currently WIP.