node-gitteh icon indicating copy to clipboard operation
node-gitteh copied to clipboard

[PATCH]: General cleanups, remove unused files, remove coffeescript

Open lushzero opened this issue 11 years ago • 8 comments

This patch cleans up unused files like lib/bindings/*, removes the dependence on coffeescript and leaves in place a static gitteh.js binding wrapper and updated the README with changes respective of those updates. Some minor changes were made to gitteh.js to improve readability and add a brief doc header, further expansion of comments in that file would be a good next step along with inline examples. I have nothing against coffeescript in general but I think the obfuscation it presents in its use here vastly outweighs the benefits it provides.

I did not cleanup the examples/ folder however most of those don't seem to work. I'm not sure in which cases they ever did, are subject to bitrot, or maybe I am doing something wrong. Someone more knowledgeable about their history should remove anything that is bitrotted, at least for the time being. No examples are better than not working examples in my opinion.

lushzero avatar Aug 26 '13 23:08 lushzero

Nice, the files I was wondering what they do are gone. But what's up with the new sgit namespace/module? Should the project be renamed too? What's the purpose behind that change?

FrozenCow avatar Aug 29 '13 14:08 FrozenCow

Oops, didn't realize the additional commits were added here automatically. I need to split that to a new repo. I intended only eec00bb for this project. After not really hearing anything and not seeing any activity I decided it would just be easier for me to fork to a totally (node-sgit, simple git) new module. When gitteh was originally written libgit was very thread unsafe and node async handling was not so good but now libgit threads are a lot better (with a couple small exceptions) so potentially things can be done in a more streamlined way and that pretty much leads to a rewrite.

lushzero avatar Aug 29 '13 19:08 lushzero

My new changes will be done in https://github.com/lushzero/node-sgit

lushzero avatar Aug 29 '13 21:08 lushzero

Oh, so then this PR should be cancel'd (or reset?), esp if the last 2 commits don't belong.

iamwilhelm avatar Aug 30 '13 00:08 iamwilhelm

eec00bb is still relevant as it removes unused files, makes the binding JS static and removes coffeescript dependencies if that is the direction the project wants to go.

lushzero avatar Aug 30 '13 01:08 lushzero

I've also had trouble with the buildscripts and found out it wasn't possible add the master version as a dependency to projects because the cakefile wasn't executed. So I also did some changes to fix this: https://github.com/FrozenCow/node-gitteh/compare/buildscripts

I wasn't sure whether it was a good idea to do a new pull request, because my changes are conflicting with yours. The change also removes Cakefile (like this PR), but moves compiling CS-to-JS to prepublish (and install when needed). It doesn't feel like the best solution though, maybe removing all CS is the nicer approach, but I thought I'd mention my change as well.

FrozenCow avatar Aug 30 '13 18:08 FrozenCow

Okay, so before trying to push this further, we should bring in a maintainer. So, @samcday, do you agree to use plain JS for these bindings, in order to remove complexity and encourage contributions? Can you? Please! :cry:

mildsunrise avatar Aug 30 '13 20:08 mildsunrise

Yea, I think it might be better. I've been looking for examples of other popular libraries to get a better idea of how it needs to be done, but most of them are way way simpler. (no coffeescript, often even no install.js script, just a gyp file it seemed) It would be good to move towards a simpler structure like that. My buildscripts-branch is already more complicated than most projects.

FrozenCow avatar Aug 30 '13 22:08 FrozenCow