-
Notifications
You must be signed in to change notification settings - Fork 11
Raise better error when configuration is missing #35
Conversation
b9d733d
to
eacf835
Compare
@codeclimate/review WDYT about getting this change in before 1.0.0? I think it's a nice little improvement to the first-time user experience |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -5,5 +5,6 @@ gem "mocha" | |||
gem "rake" | |||
gem "simplecov" | |||
gem "codeclimate-test-reporter", "1.0.0.pre.rc2" | |||
gem "pry" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal opinion: pry tends to sneak into dependencies for temporary debugging, but not actually be used long-term, so keeping it committed as a dev dependency doesn't feel appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm did a quick poll of some of our core services and we seem to have pry in slightly more than half:
builder, scheduler, api, cce: yes
wally, finalizer: no
Personally I'd be in favor. I see a benefit to having pry (find it useful when writing tests) and little downside.
Curious why you think it would
not actually be used long-term
?
By "not used long term" I guess what I really mean is that it's rarely actually present in code. It's a console/slash debugging tool (e.g
|
OK I'm going to call 🍍 and remove pry from this PR for now and encourage @ABaldwinHunter to open a new PR with pry. Personally I'm in favor of including it as well (in short: I think it needs to be in the Gemfile in order to require it when running tests via |
eacf835
to
0ad0d1e
Compare
Thanks to @ABaldwinHunter for pairing on this