yeoman / doctor

Detect potential issues with users system that could prevent Yeoman from working correctly
http://yeoman.io
BSD 2-Clause "Simplified" License
61 stars 17 forks source link

Add yo version checking #28

Closed jamen closed 8 years ago

jamen commented 8 years ago

Some releases (like patches) include fixes to bugs. Its helpful for the user to see that they are running an out-of-date version of yo, if they are.

image

jamen commented 8 years ago

I kept latest-version at 1.0.1 to be compatible with node 0.10.0

sindresorhus commented 8 years ago

Latest latest-version is compatible with Node.js 0.10: https://github.com/sindresorhus/latest-version/blob/244ac1b8d601bcfa8a38de47610a6ad8f2898712/.travis.yml#L7

sindresorhus commented 8 years ago

Still need to remove https://github.com/yeoman/doctor/pull/28/files#r52161931

jamen commented 8 years ago

You just wanted me to let it throw the error, if the package cant be accessed, instead of handling it like that? I forgot when, but I looked in the source of your packages, and I think it just originated back to package-json, which just throws a "package not found" error, or whatever error comes from got. I thought maybe I should handle it.

SBoudrias commented 8 years ago

@jamen I think it's better to let unexpected error unhandled so someone is notified when something fails.

sindresorhus commented 8 years ago

You just wanted me to let it throw the error, if the package cant be accessed, instead of handling it like that? I forgot when, but I looked in the source of your packages, and I think it just originated back to package-json, which just throws a "package not found" error, or whatever error comes from got.

Then the error message should be improved there so every consumer benefits, not here.

jamen commented 8 years ago

Alright, makes sense... But, since latest-version is Promise-based, should I throw the error?

.catch(function(err) {
  throw err;
})

Otherwise, it would probably just exit without any trace of an error happening.

jamen commented 8 years ago

I suppose that doesn't work, actually... console.log + process.exit?

SBoudrias commented 8 years ago

throw is fine as it'll be uncaught and fail the process. You should never ever use process.exit()

jamen commented 8 years ago

I'll have to use something hackish, because the promise will actually catch it again:

.catch(function(err) {
  throw err;
}).catch(function(err) {
  // Same error...  Never uncaught in the process
})

Something like:

.catch(function(err) {
  setTimeout(function() {
    throw err;
  });
})
SBoudrias commented 8 years ago

You could also use console.error(err). It'll print to stderr, so that might be good enough.

jamen commented 8 years ago

@SBoudrias: Changed it to console.error(err) (as well as call cb(err)), added tests for the new rule, fixed some travis difficulties, and squashed commits.

SBoudrias commented 8 years ago

@jamen hey, any chance we can wrap this up?

jamen commented 8 years ago

Sure thing, I will try to get your suggestions done today (tonight for me, possibly).

jamen commented 8 years ago

@SBoudrias Pretty sure it is failing to build because of stuff not related to mine now.

SBoudrias commented 8 years ago

Yeah the tests are failing due to changes inside the linter. No need to address them now.

SBoudrias commented 8 years ago

Thanks for the PR by the way! Keep em coming ;)

jamen commented 8 years ago

Your welcome, and will do.