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

grunt build system #80

Closed
wants to merge 3 commits into from
Closed

grunt build system #80

wants to merge 3 commits into from

Conversation

bsparks
Copy link
Contributor

@bsparks bsparks commented Jul 23, 2012

using grunt to build would require only node.js (not sure about the jsdocs yet, but I think it could be added). defaults to run the jshint linter, but that's optional. I plan to get all the files passing lint next. https://github.com/cowboy/grunt

@melonjs
Copy link
Collaborator

melonjs commented Jul 24, 2012

Hi,

First thank you very much, and then here is a couple of remarks :

• Grunt : I’ve been looking at it recently as well, but never gave it a try , cool addition :) Could you however include a few lines in the README on how to build melonJS with it, and maybe corresponding pre-requisites ?

• Could you further details the second one (what kind of corrections, which polyfill ? it looks scary with this “2254 additions, 2238 deletions not shown” comment and no way to view the differences, it’s not that I’m paranoid, but that’s a damn lots of changes in a single file :)

• RequestAnimationFrame polyfill : while I’m happy to use Erik Moller polyfill ( is there a specific license on that code btw), I’m not willing to revert to setTimeout as a fallback. During my previous testing (and despite of the known flaws), the advantage I found in SetInterval is that its behavior is at least common across all browsers, and I would like to keep it this way.

Can I as well ask you to redo and make separate pull request for that ?
(I won’t accept this one, and will close it if you agree)

Thanks,
Olivier.

@bsparks
Copy link
Contributor Author

bsparks commented Jul 24, 2012

Yes, I'll work on adding some stuff to the README for the grunt system. I think most of the lines are looking like changes because I converted all tabs to spaces. (one of the lint messages is "mixed tabs and spaces", tho I think using spaces instead of tabs is better). I think besides the RequestAnimationFrame one I also used the console replacement from html5 boilerplate (it's at the top of the file now). I'm fine to redo without the RequestAnimationFrame one. Most of the other changes are things like missing semi-colons, extra semi-colons, single line if blocks, other odds and ends that lint is picky about.

Anyway, I'll work on resubmitting. Thanks.

@melonjs
Copy link
Collaborator

melonjs commented Jul 24, 2012

Sorry again for being so picky as well (I was i actually feeling bad for asking you to do it again) !

@melonjs
Copy link
Collaborator

melonjs commented Jul 24, 2012

Also,
Please don't convert tab to spaces, we had some kind of bad mix last year, and it then took some time to put it back all together. Furthermore, from what I can see from other framework (JQuery) the "rule" is more to use tab than spaces.

see :
https://github.com/jquery/jquery/blob/master/.editorconfig
(actually I should do the same thing for melonJS)

@bsparks
Copy link
Contributor Author

bsparks commented Jul 24, 2012

well, hopefully this won't start a holy war, but why would you prefer tabs? The tab character is different on many systems, and also when posting code to the web. I understand if it's your project and that's what you want to do. I'm using Sublime Text 2, so it's easy for me to switch the entire document to either way (tabs or spaces).

@melonjs
Copy link
Collaborator

melonjs commented Jul 24, 2012

Hehe :) well actually I don't understand myself which one is better and I even don't really care as far as we keep it consistent over all the source code (at least try), and it's really not because it's my project (after all it's just tab or space) but most people seems to be happy with tabs and using it here and there in other projects, so I just did as well (at least tried).

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.

2 participants