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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lushzero
Copy link

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.

@FrozenCow
Copy link

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?

@lushzero
Copy link
Author

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

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

@iamwilhelm
Copy link
Contributor

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

@lushzero
Copy link
Author

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.

@FrozenCow
Copy link

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.

@mildsunrise
Copy link
Contributor

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! 😢

@FrozenCow
Copy link

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.

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.

4 participants