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

Utility methods to get Git info #314

Closed
wants to merge 5 commits into from
Closed

Conversation

SBoudrias
Copy link
Member

Follow up to issues #199 and #263

please don't merge right now!!

Async issue
I have some trouble as every Node.js utility to run command on the command line are Async (no execSync build in). ATM I added some callback, but I'm not sure that's great for a high level tool like Yeoman. I saw some execSync implementation out there (haven't test yet), but do we want to add a dependency for this? Could be good if we'd like to use npm ls to lookup installed generators, etc. Open question!

Testing
I'm usually not so bad at getting tests right, but right now they are quite bad. I'm really not sure how to handle these as the method go get user config (and as just mocking the exec method seems pretty weak in term of added value). I'll think about it more, but I'm open to others idea and expertise!

Anyway, that'll get the ball rolling!

@SBoudrias
Copy link
Member Author

BTW, most execSync implementations seems a little unmaintained right now. There's shelljs who's pretty cool (and do much more). It've already been cited on issue #190 by @robwierzbowski (any feedback on shelljs?)

@robwierzbowski
Copy link

I've had no problems with shell.js, although I haven't done much with it besides what's going on in generator-jekyllrb. I started with execSync, but switched because of a couple missing features and the lack of maintenance.

@sindresorhus
Copy link
Member

Would it be possible to use js-git? That would remove the exec requirement.

Problem with all execShell implementation is that they require native bindings which means having to compile, which is slow and could fail on some systems.

Btw, you guys should add your thoughts here: nodejs/node-v0.x-archive#5358 We really need execSync in Node core.

@addyosmani
Copy link
Member

I wouldn't mind us using shelljs.

@sindresorhus do you have a good feel for how stable js-git is? I could be wrong but on preliminary glances it doesn't look like a lot of projects are using it just yet. Unsure of whether that is down to need or waiting on stable releases however.

@sindresorhus
Copy link
Member

do you have a good feel for how stable js-git is? I could be wrong but on preliminary glances it doesn't look like a lot of projects are using it just yet. Unsure of whether that is down to need or waiting on stable releases however.

Not sure, but getting the git config is such a simple operation. Could at least look into using it for this.

@SBoudrias
Copy link
Member Author

Hoy hoy!

Got a working synchronous version.

1- I added shelljs as an independent core module (just like lodash). I hesitated to wrap around shelljs module and apply partials to silent the output by default - finally haven't made it, but I'd like your opinion. (I haven't gone for it in the end as I didn't want to break anything when functions take different number of parameters)

2- Added the User module so it is there for future needs, and the git methods are inclosed in this module. I also added way better unit tests thanks to shelljs super cool extra functions (it would've worked in bare bone Node.js too, but it is nice to have and read).

Let me know what you thinks!

@@ -0,0 +1,3 @@
var shell = require('shelljs');

module.exports.shell = shell;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a separate file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just for concern separation, but I'm open to suggestion.

I hesitated to merge this with spawn_command as they're both related to process running in a shell.

Copy link
Member

Choose a reason for hiding this comment

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

Just require shelljs directly in user.js and base.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

That could work. I just based myself on actions/string.js to keep consistency. This module does mostly the same thing as I did in actions/shell.js (less a little logic to merge Lodash and underscore.string).

So maybe it'll be best to stick to a style?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated for the current. Might be good to consider it for the string module too in the future.

@SBoudrias
Copy link
Member Author

I'm wondering if the module should really be named user or naming it git would be better, e.g.

this.user.gitUsername
this.git.username

@robwierzbowski
Copy link

If there's a chance that this module will provide user information by other methods than querying the .gitconfig I think user is a better choice.

…nd implement cache. Also refactoring how shell module is publicly exposed.
@SBoudrias
Copy link
Member Author

OK, updated the API to use value getters with a cache: this.user.git.email and this.user.git.username.

@sindresorhus
Copy link
Member

Thanks @SBoudrias ! Nice work. Landed.

@sindresorhus
Copy link
Member

🍰

sindresorhus added a commit that referenced this pull request Jul 14, 2013
Use plain object getters and code style
@sindresorhus sindresorhus mentioned this pull request Jul 14, 2013
6 tasks
@passy
Copy link
Member

passy commented Jul 14, 2013

Great work! 🍰

@robwierzbowski
Copy link

Correct me if I'm wrong, but I don't think this provides a method of getting the GitHub username. That was one of the goals in #199, and also one of the things I want to use it for 😄. Removed for a reason or oversight?

A little late to be making suggestions, but pulling in the whole .gitconfig as a javascript object and letting the user drill into it might be more future proof, e.g.:

user.git.user.name;
user.git.user.email;
user.git.github.user;

I don't know if people store sensitive information in their .gitconfig (I don't), but it's all available with shelljs anyways.

@addyosmani
Copy link
Member

I wouldn't be opposed to us pulling in the .gitconfig as an object. Afaik most people just use it to also store name, email, aliases etc and it shouldn't be that risky to pull in.

@SBoudrias
Copy link
Member Author

@robwierzbowski Under which circumstance is the github username stored in the .gitconfig - I've never seen this.

@robwierzbowski
Copy link

Now that I look it up, it's probably either something I added for Hub or when I was committing under different names on GitHub and Drupal.org. I've had it in there for so long I assumed it was official. It's also referenced in 199, so maybe it's widespread.

Since it's not a standard key, and since it's the only other info I'd like from the config file, we can probably leave things as they are.

@kevva
Copy link
Member

kevva commented Sep 2, 2013

We can use the email and pull the username from that.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7c24054 on SBoudrias:git into * on yeoman:master*.

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.

None yet

7 participants