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

fix(generator): fix tdd option #40

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

fxleblanc
Copy link

No description provided.

@@ -0,0 +1,11 @@
(function () {
Copy link
Member

Choose a reason for hiding this comment

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

Why wrapping the tests in a IIFE? There's no reason to do so

Copy link
Author

Choose a reason for hiding this comment

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

You're right. Should I also remove the use strict instruction?

Copy link
Member

Choose a reason for hiding this comment

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

No, use strict is still important.

@SBoudrias
Copy link
Member

cc @eddiemonge

@eddiemonge
Copy link
Member

I'm conflicted on this. While using the correct language is good, how many people actually use the suite and test syntax?

Also, having diverging templates means having to change both instances when there is a change. In this case, I don't really see that happening a lot, if ever.'

A third point is that it locks the style to TDD or BDD. While running this directly is not a problem, generators that compose on top of this may break if they previously specified QUnit or something else.

@fxleblanc
Copy link
Author

@eddiemonge I agree, updating the templates is not a good way of doing this. We could always search and replace if tdd is specified.

@fxleblanc
Copy link
Author

For my part, I always use the bdd way. We could remove the tdd option entirely. It's your call.

@eddiemonge
Copy link
Member

If it can be done in one template I'd be ok with this

@SBoudrias
Copy link
Member

I'd either drop the --ui option, or keep two templates. if/else inside templates are the worse to maintain.

@fxleblanc
Copy link
Author

We could leverage the ejs templates in the test file.

<%= ui.suite %>('suite', function() {
  <%= ui.test %>('test', function(done) {
    assert.ok(true);
  }); 
});

This snippet is theoratical but this is what I have in mind.

Base automatically changed from master to main January 24, 2021 06:35
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