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

replaced git-standup with gitlog #18

Merged
merged 26 commits into from
May 14, 2017
Merged

replaced git-standup with gitlog #18

merged 26 commits into from
May 14, 2017

Conversation

mojoaxel
Copy link
Collaborator

@mojoaxel mojoaxel commented Apr 18, 2017

I replaced git-standup with gitlog.
This should solve issues #11 and #14 🎉
This is an alternative to #8.

@mojoaxel
Copy link
Collaborator Author

mojoaxel commented Apr 18, 2017

This should also fix #17

@madve2
Copy link

madve2 commented Apr 19, 2017

The nice thing about this is that it also works on Windows! Yay, thank you!

package.json Outdated
@@ -4,6 +4,9 @@
"description": "A terminal that tries to take care of you 💖",
"main": "care.js",
"author": "Monica Dinculescu <[email protected]>",
"contributors": [
Copy link
Owner

Choose a reason for hiding this comment

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

I haven't been doing this for anyone else who has been sending PRs, so I'm inclined to just have the GitHub contributors reflect this. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem at all! Let's keep the package.json slim 😉

@notwaldorf
Copy link
Owner

I tried running your patch locally, but I get:

Error: Repo location does not exist
    at gitlog (/Users/noms/Code/tiny-terminal-code/node_modules/gitlog/index.js:59:11)
    at async.each (/Users/noms/Code/tiny-terminal-code/care.js:124:5)
    at /Users/noms/Code/tiny-terminal-code/node_modules/async/dist/async.js:3047:16

@notwaldorf
Copy link
Owner

notwaldorf commented Apr 19, 2017

Ohhh I see. So the advantage of git-standup is that you can just pass a directory that contains a bunch of other repos (like ~/Code contains all my projects), and it would just crawl through them. With this approach, it looks like I have to list every repo? I have about 140, so I don't think that would work 😂

@mojoaxel
Copy link
Collaborator Author

mojoaxel commented Apr 19, 2017

With this approach, it looks like I have to list every repo? I have about 140, so I don't think that would work

Good, point! I see if I can improve further... 🤘

@notwaldorf notwaldorf added the ⏳ pending-response What are you waiting for? Christmas? label Apr 19, 2017
@mojoaxel
Copy link
Collaborator Author

@notwaldorf I introduced a new env var TTC_REPOS_DEPTH to define the max directory-depth.

package.json Outdated
"parrotsay-api": "^0.1.1",
"scraperjs": "^1.2.0",
"subdirs": "github:mojoaxel/subdirs",
Copy link
Collaborator Author

@mojoaxel mojoaxel Apr 20, 2017

Choose a reason for hiding this comment

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

This is not perfect. I'm waiting for jarofghosts/subdirs#2 to get merged and published...

@mojoaxel
Copy link
Collaborator Author

@notwaldorf Ok, I now also updated the README and introduced some error handling ✨.

Now you have to decide if you want to get rid of the script-based git-standup solutions in favor of this pure-js implementation. If you want to stick with git-standup that's also OK with me, just close this PR than (no hard feelings).

@madve2
Copy link

madve2 commented Apr 21, 2017

I really hope this will be the solution going forward, because the git-standup script version will never work on Windows.

@Garbee
Copy link
Contributor

Garbee commented Apr 21, 2017

If Windows support with the helper script is needed that is certainly simple enough to be ported into a Windows script that can be executed based on the environment. No need to convert the entire subsystem over just for that.

@madve2
Copy link

madve2 commented Apr 21, 2017

Are you sure about that? Replacing a dependency with a similar one (e.g. git-standup => gitlog) is hardly "convert[ing] the entire subsystem", and there are clear benefits in having a platform-independent, pure JS solution over having platform specific scripts written for different environments. (That's the whole point of node.js after all.) Yes, PowerShell is perfectly capable of what the current shell script is doing, but is a ps1 script really worth writing and maintaining? Especially considering the burden of having to update and test both scripts from now on for the rest of the project's lifetime, which won't be easy as it will require testing on at least two different operating systems.

@Garbee
Copy link
Contributor

Garbee commented Apr 21, 2017

I never said it was a bad change, I simply was stating if Windows support is desired then this effort of changing the way the git logs are retrieved (the subsystem) is not entirely required. The script is simple enough to do in a native Windows shell to at least get some initial support until this solution can be more thoroughly looked over. From some experience in the Firefox's new HTML-based Debugger for their DevTools, handling git stuff well and in a compatible way through Node isn't always straightforward. Some care should be taken before rushing into it and then having quirky setups cause issues that need to be solved that otherwise aren't issues.

care.js Outdated
* and look for repositories.
* Calls `callback` with array of repositories.
*/
function getGitRepos(repos, depth, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like PR #36 may land opening up more ES6 usage. It may be good to wait for that to land then refactor these extra functions into isolated modules to keep the main care file smaller.

care.js Outdated
var blessed = require('blessed');
var contrib = require('blessed-contrib');
var chalk = require('chalk');
var parrotSay = require('parrotsay-api');
var bunnySay = require('sign-bunny');
var weather = require('weather-js');

console.dir(gitbot);
Copy link
Owner

Choose a reason for hiding this comment

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

remove :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😱 sorry...gone

care.js Outdated
@@ -1,15 +1,18 @@
#!/usr/bin/env node
var config = require(__dirname + '/config.js');
var twitterbot = require(__dirname + '/twitterbot.js');
var gitbot = require(__dirname + '/gitbot.js');
Copy link
Owner

Choose a reason for hiding this comment

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

🙌

care.js Outdated
}

gitbot.findGitRepos(config.repos, config.reposDepth-1, (err, allRepos) => {
if (err) return showError(err);
Copy link
Owner

Choose a reason for hiding this comment

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

return on a new line, please (here and everywhere)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

noproblemo

care.js Outdated
}

function showError(err, box) {
getCommits(`😥 ${err}`, box);
Copy link
Owner

Choose a reason for hiding this comment

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

Is this bit actually needed? For an error, can't we just do box.content = err?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea!

config.js Outdated
repos: (process.env.TTC_REPOS || '~/Code').split(','),

// The directory-depth on how to look for git-repos. Use with care!
reposDepth: process.env.TTC_REPOS_DEPTH || 1,
Copy link
Owner

Choose a reason for hiding this comment

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

this is already called depth, below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry...merges and stuff...will fix

@mojoaxel
Copy link
Collaborator Author

I'll add the repos to the commits and think about a nice way for the transition!

@mojoaxel
Copy link
Collaborator Author

mojoaxel commented Apr 27, 2017

@notwaldorf I added the repo names. I also added a var 'TTC_GITBOT' to switch between git-standup and the node implementation. Both methods seem to work on linux, hopefully also on mac.

@mojoaxel
Copy link
Collaborator Author

mojoaxel commented Apr 27, 2017

Interesting: git-standup seems to filter out merge-commits like e.g. this one: Merge branch 'master' into gitlog.
So both implementations do not result in the same commits at the moment. Not sure if that's a bug or a feature 😕

@mojoaxel
Copy link
Collaborator Author

Update: I now also filter pure administrative commits like merges (gitlog has a attribute for that).

Copy link
Owner

@notwaldorf notwaldorf left a comment

Choose a reason for hiding this comment

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

I think the gitlog is aces, and works great for me. My only concern is that along with adding gitlog, this PR also changes the current behaviour of git-standup, which means if people don't switch to gitlog they might have new git-standup problems they didn't have this PR, and I'd really like to not have that

care.js Outdated
if (config.gitbot.toLowerCase() === 'gitstandup') {
todayBox.content = '';
config.repos.forEach(repo => {
var today = spawn(`cd ${repo} && git-standup`, [`-m ${config.depth}`, '-d 1'], {shell:true});
Copy link
Owner

Choose a reason for hiding this comment

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

uhhhhhh so I'm not 100% sure this works, because git-standup is on the wrong path (and works locally, but doesn't work when installed as a package). Can you just revert this, and leave the old git-standup behaviour, so that this PR just adds gitlog, and not other things? That way it's easier if something goes wrong after this PR :)

care.js Outdated
});
weekBox.content = '';
config.repos.forEach(repo => {
var today = spawn(`cd ${repo} && git-standup`, [`-m ${config.depth}`, '-d 7'], {shell:true});
Copy link
Owner

Choose a reason for hiding this comment

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

same here

care.js Outdated
} else {
gitbot.findGitRepos(config.repos, config.depth-1, (err, allRepos) => {
if (err)
return todayBox.content = err;
Copy link
Owner

Choose a reason for hiding this comment

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

this won't call screen.render() anywhere, will it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point! I'll improve..


// Directory-depth to look for git repositories.
depth: (process.env.TTC_REPOS_DEPTH || 1),

// Which method is to be used to read the git commits ('gitstandup' | 'gitlog').
gitbot: (process.env.TTC_GITBOT || 'gitstandup'),
Copy link
Owner

Choose a reason for hiding this comment

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

👍 Awesome!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

git-standup is the default settings for now. Let's give the gitlog implementation a try on different platforms, maybe we can remove the whole bash-scripting stuff all together at some point!

package.json Outdated
"twit": "^2.2.5",
"weather-js": "^2.0.0"
},
"peerDependencies": {
Copy link
Owner

Choose a reason for hiding this comment

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

pls add this back in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@notwaldorf Wow! You actually reviewed this at #jsconfeu ! 🎉 Thx. I'll try to have a look tonight.

@mojoaxel
Copy link
Collaborator Author

mojoaxel commented May 7, 2017

@notwaldorf Yo! Check this out! ;-)
(I was a little side-tracked from @fhinkel's talk - hope it works!)

@notwaldorf
Copy link
Owner

Awesome! YAY! Thanks!

@notwaldorf notwaldorf merged commit 3ad069c into notwaldorf:master May 14, 2017
@mojoaxel
Copy link
Collaborator Author

🎉 🎈 !

@mojoaxel mojoaxel deleted the gitlog branch May 14, 2017 15:16
@KernelDeimos
Copy link

KernelDeimos commented May 14, 2017

I just cloned the latest changes and received a new error :D (sorry, my computer is probably perfectly screwed up for this, lol)

Any idea what might be causing this?
screenshot_2017-05-14_14-16-24

I'm going to add my steps for running in case that changes something:

  1. Add export TTC_GITBOT=gitlog to .profile and source ~/.profile
  2. Clone the repo
  3. cd into repo and run npm install
  4. run npm start

@notwaldorf
Copy link
Owner

@KernelDeimos What's your node version? It seems like it isn't loving the ES6 keywords.

@mojoaxel
Copy link
Collaborator Author

mojoaxel commented May 14, 2017

Any idea what might be causing this?

Wow! Please update node.js to >= 6.10.3

@mojoaxel mojoaxel mentioned this pull request May 14, 2017
@KernelDeimos
Copy link

KernelDeimos commented May 15, 2017

My bad - I thought Ubuntu's repos would have it at least reasonably up-to-date :/ boy was I wrong

Side note: Apparently I can't upgrade node.js without running some shell script from the Internet as root. I haven't found a single reference online that tells me I can just add something to my sources file... what's up with that?

@Garbee
Copy link
Contributor

Garbee commented May 17, 2017

I haven't found a single reference online that tells me I can just add something to my sources file... what's up with that?

The only stable way to get a newer version without root access is Node Version Manager. It installs node versions into your user folder with your permissions. The os-level update from an external repository will need root access since in those cases you are editing your global software sources list and installing the new version.

Generally running trusted scripts as root isn't a problem. You can always download the script and look it over before running it to see what it is doing. If you don't have root access (which is fairly uncommon, especially on workstation machines) then you'll need to go the NVM route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏳ pending-response What are you waiting for? Christmas?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants