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

Set Yadda options #72

Merged
merged 6 commits into from
Mar 15, 2018
Merged

Set Yadda options #72

merged 6 commits into from
Mar 15, 2018

Conversation

benwalder
Copy link
Collaborator

@benwalder benwalder commented Mar 12, 2018

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:

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {

    'ember-cli-yadda': {
      yaddaOptions: { // passed through to yadda parseFeature()
        language: 'Polish', // converted to Yadda.localisation.Polish
        leftPlaceholderChar: '<',
        rightPlaceholderChar: '>'
      }

    }
  });

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.

@albertjan
Copy link
Collaborator

LGTM

Copy link
Member

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

👍

@benwalder
Copy link
Collaborator Author

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. ❓

@simonihmig
Copy link
Member

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!

@benwalder
Copy link
Collaborator Author

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.

Copy link
Member

@simonihmig simonihmig left a 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!');
Copy link
Member

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!

@simonihmig
Copy link
Member

I have manually tested the qunit side, but not mocha. Also need to consider how to add any automated tests.

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...

@benwalder benwalder changed the title WIP: Set Yadda options Set Yadda options Mar 15, 2018
@benwalder
Copy link
Collaborator Author

@albertjan @simonihmig Ready to merge and publish 0.4.0?

@simonihmig
Copy link
Member

Yes, go for it! 🎉

@benwalder benwalder merged commit aed21db into kaliber5:master Mar 15, 2018
@albertjan
Copy link
Collaborator

Nice one guys! Does one of you want to do the release tweet? Scream and shout about what we've done 😛?

@benwalder benwalder deleted the yadda-options branch March 15, 2018 15:50
@benwalder
Copy link
Collaborator Author

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.

@simonihmig
Copy link
Member

@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 ember-bootstrap, that has its test written with the new testing APIs, and it is tested with ember-try down to Ember 2.3! You just have to use the new setup functions and use the test helpers from @ember/test-helpers.

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 moduleFor... functions anymore, as module() is hardcoded in the test runner...

Just for clarification ;)

@simonihmig
Copy link
Member

Nice one guys! Does one of you want to do the release tweet? Scream and shout about what we've done 😛?

The privilege belongs to the maintainer I think!? 😉
But if you prefer, I can certainly do that as well...

@benwalder
Copy link
Collaborator Author

it is tested with ember-try down to Ember 2.3!

😳 😫 😬 duh, thanks for the fix.

@albertjan
Copy link
Collaborator

The privilege belongs to the maintainer I think!? 😉
But if you prefer, I can certainly do that as well...

haha, you guys did all the work! I'll retweet! 😄

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.

3 participants