Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

Rebase on #306 and additions #518

Conversation

stephanebachelier
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@stephanebachelier
Copy link
Collaborator Author

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

@stephanebachelier
Copy link
Collaborator Author

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.

@@ -255,6 +255,67 @@ useminPrepare: {

The given steps or post-processors may be specified as strings (for the default steps and post-processors), or as an object (for the user-defined ones).

### warnMissing
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need an option for this?

@XhmikosR
Copy link
Contributor

@stephanebachelier: please rebase.

@stephanebachelier
Copy link
Collaborator Author

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

@stephanebachelier
Copy link
Collaborator Author

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

@XhmikosR
Copy link
Contributor

XhmikosR commented Mar 4, 2015

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

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

### resolveSource

Type: 'function'
Default: `nil`
Copy link
Contributor

Choose a reason for hiding this comment

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

nil?

@stephanebachelier
Copy link
Collaborator Author

@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:

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
Copy link
Collaborator Author

friend ping @XhmikosR could you review this PR ?

@XhmikosR
Copy link
Contributor

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
Copy link
Collaborator Author

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

@stevemao
Copy link
Contributor

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
Copy link
Collaborator Author

@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
Copy link
Contributor

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

Document the fact that both usemin and useminPrepare now throw an error
on resource not found
@stephanebachelier
Copy link
Collaborator Author

@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
Copy link
Member

Looks ok.

@stephanebachelier
Copy link
Collaborator Author

Closing in favor of #535.

@stephanebachelier stephanebachelier deleted the eli-colins-works-rebased branch April 12, 2015 09:46
@schmod schmod mentioned this pull request May 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn on missing files
4 participants