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

Reorder grunt tasks, remove the Procfile, remove grunt-shell dependency #311

Merged
merged 5 commits into from
Sep 19, 2016

Conversation

gemmaleigh
Copy link
Contributor

@gemmaleigh gemmaleigh commented Sep 12, 2016

What does it do?

This PR:

  1. makes the grunt tasks less verbose and reorders them, to make them easier to read
  2. removes the Procfile [*]
  3. removes the dependency on grunt-shell and instead uses npm scripts

[*] If no Procfile is present in the root directory of your app during the build process, your web process will be started by running npm start, specified in package.json.

What type of change is it?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Put an x in all the boxes that apply

  • I have read the CONTRIBUTING document.

@gemmaleigh gemmaleigh force-pushed the add-test-for-procfile branch from 29383a9 to 8ed1a0d Compare September 12, 2016 13:42
'lint',
'Use govuk-scss-lint to lint the sass files',
'test',
'',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be empty?

@gemmaleigh gemmaleigh force-pushed the add-test-for-procfile branch from b60011e to 06b820e Compare September 12, 2016 13:55
'encode_snippets',
'Encode HTML snippets',
function () {
grunt.task.run('htmlentities')
}
)

// Test
grunt.registerTask(
Copy link
Contributor

@NickColley NickColley Sep 12, 2016

Choose a reason for hiding this comment

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

Might be my ignorance but do these need to be this verbose?

http://gruntjs.com/api/grunt.task

e.g.

task.registerTask('test', ['jshint', 'test_default', 'test_default_success']);

@gemmaleigh
Copy link
Contributor Author

cc. @robinwhittleton, @NickColley.

This also includes the removal of the grunt-shell dependency, to match the govuk_frontend_toolkit.

@gemmaleigh gemmaleigh changed the title Reorder the grunt test tasks and remove the Procfile Reorder grunt tasks, remove the Procfile, remove grunt-shell dependency Sep 19, 2016
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

LGTM 👍

}
)
// Encode HTML snippets
grunt.registerTask('encode_snippets', ['htmlentities', 'htmlentities_success'])
Copy link
Contributor

@NickColley NickColley Sep 19, 2016

Choose a reason for hiding this comment

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

Minor: You don't expect to see underscores in idomatic JavaScript that much so it feels odd to see encode_snippets instead of encode-snippet etc but it's not a big deal

@gemmaleigh gemmaleigh force-pushed the add-test-for-procfile branch from ed21b72 to 7d284b2 Compare September 19, 2016 12:42
- Add comments to each of the test tasks
- Make tasks less verbose
- Rename Grunt tasks to use hyphens not underscores
Also use standard —fix to fix indentation.
https://devcenter.heroku.com/articles/nodejs-support#default-web-process
-type

If no Procfile is present in the root directory of your app during the
build process, your web process will be started by running npm start, a
script you can specify in package.json.
This then outputs `npm ERR! Test failed.  See above for more details.`,
rather than the more verbose error.
@gemmaleigh gemmaleigh force-pushed the add-test-for-procfile branch from 7d284b2 to 43df27c Compare September 19, 2016 12:47
@gemmaleigh gemmaleigh force-pushed the add-test-for-procfile branch from 8c76080 to 38b1f24 Compare September 19, 2016 12:57
@robinwhittleton robinwhittleton merged commit b460612 into master Sep 19, 2016
@robinwhittleton robinwhittleton deleted the add-test-for-procfile branch September 19, 2016 14:31
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