Rebase on #306 and additions

[stephanebachelier]

This is a rebase of #306 with an addition that make useminPrepare and usemin to warn if no html is found.

This PR should then resolved #260, #404, #452.

[stephanebachelier]

Hmm tests are failing on travis not on my machine. Will investigate the issue.

[stephanebachelier]

Ok I can reproduce the errors. I removed all node_modules and make a fresh npm install.

[stephanebachelier]

ok now it's fixed. The difficult part to not throw error and instead using grunt.fail.warn which let user decide if they want to force or not the execution. IMO it's better to let user decide than crash the process.

For the test I've stubbed grunt.fail.warn when needed and restored at the end, so it should be ok. It's the cleanest approach I've found.

Two test files are using this stub:

• test-config-writer: this should not be a problem as it does not use grunt
• test-usemin: this could be a problem because these tests are run with grunt but the stub being added around just before the failing code and immediately restored it should not be an issue.

CC @sindresorhus for code review.

[sindresorhus]

Do we really need an option for this?

[XhmikosR]

@stephanebachelier: please rebase.

[stephanebachelier]

@XhmikosR what's your position on the warnMissing option ?

[stephanebachelier]

FYI I've begin to play with defaulting warnMissing to true. Need to fix some tests.

[XhmikosR]

Generally, I think there should be a warning when a source file is missing. Now, adding a separate option for that seems a little bit too much.

[stephanebachelier]

@XhmikosR I will rework this PR to remove warnMissing option, defaulting to true is lot of work mainly test to fix, for no real gain.

[stephanebachelier]

@XhmikosR if you want to review. I've updated the PR to remove the warnMissing option. By the way the original author added support for throwing an error if a resource is missing, a feature ask by lots of folks, thus I add to refactor the tests.
I found that the mocking part of the filesystem was not correctly cleaned after each test which explains why the refactoring is important.

[XhmikosR]

@stephanebachelier: Why so many commits? And what's with the (foo) thingy? That just seems bloat... One last thing, I don't see as author @eli-collins anywhere.

Anyway, regarding the PR itself, the changes are so many it needs a lot of time to carefully review. This should be split somehow so that it's smaller and easier to review, but if you are confident about the changes, your call.

[XhmikosR]

nil?

[stephanebachelier]

@XhmikosR the (foo) thingy convention is the one from AngularJS git commit convention that you can find in angularjs repo. The idea is to generate changelog and moreover recognize important commits.

The original author of this PR is @eli-collins in #306. As I'm working on triaging and fixing issues grunt-usemin issues, I've taken its original work, rebased on master and made the adjustment you asked (remove warnMissing option and refactored the tests).

To ease the code review and possible rebase, I've decided to not squash commits. The rebase of #306 was really painful.

I know it's a huge PR but it's mainly two parts:

• enhanced source path resolution for useminPrepare #306 work rebased, with docs and some tests
• then remove the warnMissing option that enhanced source path resolution for useminPrepare #306 brought, which meant to fix the tests.

The second part was also painful because there was no cleaning of file system after each existing tests, which is not great.

I'm quite confident with this PR. I've carefully reviewed it, add more tests but it will be great if someone is willing to review @eli-colins and my work. I might have missed something, and it might create some issues, but I'm active on the project as you can see. There are so many issues opened, and this PR will solve at least 3 annoying issues.

As there is no more active maintainer on this project, see #313 and @sindresorhus comment, I've started to triage issues and fix them.

There are lots of issues that I hope to fix by migrating from regexp to an html parser. Work is already on the go, and it will probably be a bigger PR.

If you want to discuss about it, it might be better out of this issue :)

[stephanebachelier]

friend ping @XhmikosR could you review this PR ?

[XhmikosR]

Sorry it's pretty hard to review this :/ if tests pass I guess it's OK and
people will complain otherwise.
On Mar 19, 2015 11:00 PM, "Stéphane Bachelier" [email protected]
wrote:

friend ping @XhmikosR https://github.com/XhmikosR could you review this
PR ?

—
Reply to this email directly or view it on GitHub
#518 (comment).

[stephanebachelier]

@XhmikosR This PR fix some issues so I think it's a good tradeoff and I could support any user complaining.

[stevemao]

Why do you keep the initial commits that include "Default: nil" in the readme? Since the commits are not squashed, you could just make up the perfect commits that's more logical.

[stephanebachelier]

@stevemao As already said I've manually imported commits from #306 and rebased them on master. The default nil value from the README were just something that I did not see.
As I might squash all the commits after this PR ever get reviewed I don't think this is a problem.

[stevemao]

Ok, I just thought it might be harder to review with too many commits.. Good luck.

[stephanebachelier]

@XhmikosR @stevemao I've finally changed my mind and squash all the commits. It should be easier to review.

@XhmikosR @stevemao If nobody wants to make a code review will you be ok if I create a dev branch to merge this branch and a few others PR that are waiting. These should fix some issues and it would help some of our users. I would also like to test this dev branch on some yeoman generators.

/cc @sindresorhus I know you don't have time to work on this project anymore, but your opinion would help.

[sindresorhus]

Looks ok.

[stephanebachelier]

Closing in favor of #535.