Code review - Issue 6842104: Ignore editor files.https://codereview.appspot.com/2012-11-28T19:50:53+00:00rietveld
Message from unknown
2012-11-27T18:27:21+00:00matthew.scotturn:md5:8413f72ffae0b75f5f3315f137c6bafc
Message from matthew.scott@canonical.com
2012-11-27T18:27:24+00:00matthew.scotturn:md5:93ddca1cea6f4dd6f21d0d2bd551420f
Please take a look.
Message from benji.york@gmail.com
2012-11-27T21:08:15+00:00benjiurn:md5:dab47c80a75e2edbaae2ef54c898d08d
Thanks for wanting to make the world a cleaner place. :)
The better way to accomplish this is to modify your local .bazaar/ignore file.
This is better both because we do not have to handle every type of editor or other tool that generates temp files used by everyone on the project and because the configuration is done once per developer and used across all projects that the person works on instead of having to be repeatedly configured one project at a time.
Message from gary.poster@canonical.com
2012-11-27T21:09:10+00:00gary.posterurn:md5:3dd9970c4051da966d5a3f7555e87b15
+1. Thanks, Matt
Gary
Message from benji.york@gmail.com
2012-11-27T21:18:36+00:00benjiurn:md5:b8f5d366c498110bfb9599006eb325ef
On 2012/11/27 21:08:15, benji wrote:
> Thanks for wanting to make the world a cleaner place. :)
>
> The better way to accomplish this is to modify your local .bazaar/ignore file.
>
> This is better both because we do not have to handle every type of editor or
> other tool that generates temp files used by everyone on the project and because
> the configuration is done once per developer and used across all projects that
> the person works on instead of having to be repeatedly configured one project at
> a time.
I failed to realize the true intent of the branch. I now understand (I
hope) and would like to suggest that the file watcher should take the
same approach as the Makefile, that is, it should only concern itself
with files which bzr manages. The Makefile shows how to get that
information.
Message from unknown
2012-11-27T22:19:26+00:00matthew.scotturn:md5:4e718f60377eb9403cf7f755a506388b
Message from matthew.scott@canonical.com
2012-11-27T22:19:27+00:00matthew.scotturn:md5:48e3c0ac9b91116ed9d487c19a9e79c0
Please take a look.
Message from gary.poster@canonical.com
2012-11-27T22:45:07+00:00gary.posterurn:md5:5508e640c45f6ab73ee9615b7748b6cb
Looks good to me. After changes requested, please land!
Thank you.
Gary
=== modified file 'lib/templates.js'
--- lib/templates.js 2012-11-27 22:18:41 +0000
+++ lib/templates.js 2012-11-27 22:43:52 +0000
@@ -131,7 +131,7 @@
* @param {function} cb Callback to call if the file is valid.
*/
function ifValidFile(fullpath, cb) {
- exec('bzr file-id ' + fullpath + ' > /dev/null 2>&1',
+ exec('bzr file-id ' + fullpath,
function(error, stdout, stderr) {
if (error === null) {
cb();
@@ -190,7 +190,7 @@
function watchTemplates(cb) {
fs.watch(config.server.template_dir, function(event, filename) {
//on dir change regen the cache
- ifValidFile(path.join(config.server.view_dir, filename), function() {
+ ifValidFile(path.join(config.server.template_dir, filename), function() {
var strategy = templateSpecs.templates;
strategy.callback(strategy, 'templates');
if (cb) {cb();}
https://codereview.appspot.com/6842104/diff/3002/lib/templates.js
File lib/templates.js (right):
https://codereview.appspot.com/6842104/diff/3002/lib/templates.js#newcode128
lib/templates.js:128: * are files created by vim and emacs; feel free to add to this list.
You'll need to adjust this comment, of course.
https://codereview.appspot.com/6842104/diff/3002/lib/templates.js#newcode133
lib/templates.js:133: function ifValidFile(fullpath, cb) {
Nice naming
https://codereview.appspot.com/6842104/diff/3002/lib/templates.js#newcode134
lib/templates.js:134: exec('bzr file-id ' + fullpath + ' > /dev/null 2>&1',
you can leave out the /dev/null bit, for cleanliness. The fact that we ignore stdout and stderr in the callback below should be effectively equivalent, unless the command generates so much blather that we run out of memory or something.
https://codereview.appspot.com/6842104/diff/3002/lib/templates.js#newcode147
lib/templates.js:147: return true;*/
(You clarified on IRC that you will remove this bit if we go down this path)
https://codereview.appspot.com/6842104/diff/3002/lib/templates.js#newcode193
lib/templates.js:193: ifValidFile(path.join(config.server.view_dir, filename), function() {
config.server.template_dir
Message from unknown
2012-11-28T19:48:31+00:00matthew.scotturn:md5:10c6fd8020b44d9ae67f446d31dc840b
Message from matthew.scott@canonical.com
2012-11-28T19:50:53+00:00matthew.scotturn:md5:654d5264094657cf1f1edee98f2e8e12
*** Submitted:
Ignore editor files.
Watch functions will throw noisy errors on files not part of the distribution, making debugging watched files (templates, LESS stylesheet) difficult in `make debug`; an ifValidFile function was created to ignore these and slim down useless errors. Quick slack task.
R=benji, gary.poster
CC=
https://codereview.appspot.com/6842104