capistrano / rbenv

Idiomatic rbenv support for Capistrano 3.x
MIT License
203 stars 60 forks source link

Update validation message if rbenv_ruby is not set #70

Closed ivanovaleksey closed 7 years ago

ivanovaleksey commented 7 years ago

Hi, I use new capistrano/rbenv feature that allows not to define rbenv_ruby in config file. But the warning message wasn't updated and now it seems a little bit confusing to me.

If I intentionally removed rbenv_ruby from capistrano config "rbenv: rbenv_ruby is not set" looks like I still do something wrong.

It is already said in README

Alternatively, allow the remote host's rbenv to determine the appropriate Ruby version by omitting :rbenv_ruby. This approach is useful if you have a .ruby-version file in your project.

So I think it would be better to add this notice directly to the message.

P. S. I am not sure about words in the message. Is remote hosts better than deployed hosts?

capistrano-bot commented 7 years ago
1 Warning
:warning: Please update CHANGELOG.md with a description of your changes. If this PR is not a user-facing change (e.g. just refactoring), you can disregard this.

Thanks for the PR! This project lacks automated tests, which makes reviewing and approving PRs somewhat difficult. Please make sure that your contribution has not broken backwards compatibility or introduced any risky changes.

If you have not already done so, consider including output of your code running in a production environment as "proof" that the PR works as intended. This will make it much more likely that your changes are accepted.

Thanks again for taking the time to improve Capistrano! 😃

Here's an example of a CHANGELOG.md entry (place it immediately under the * Your contribution here! line):

* [#70](https://github.com/capistrano/rbenv/pull/70): Update validation message if rbenv_ruby is not set - [@ivanovaleksey](https://github.com/ivanovaleksey)

Generated by :no_entry_sign: Danger

mattbrictson commented 7 years ago

@ivanovaleksey Good point! Thanks for the PR.

@spikeheap Do you have an suggestion for if/how we should change the warning? It seems weird to warn the user when they are doing nothing wrong.

ivanovaleksey commented 7 years ago

I have updated the message a little bit:

Shoudn't it be ... defined by the remote hosts instead of ... defined on remote hosts?

spikeheap commented 7 years ago

Hi @ivanovaleksey, this PR looks good, particularly downgrading the message to info. @mattbrictson is spot on about warning users when they're doing the 'right' thing.

This does alter the initial behaviour, which required rbenv_ruby to be set, but IMHO this is more intuitive behaviour for the uses of rbenv I've seen.

👍

ivanovaleksey commented 7 years ago

@spikeheap thank you, but should I use on or by? And should I use definite article? Survey time 🎉

spikeheap commented 7 years ago

Hi @ivanovaleksey, I do love to bikeshed, so...

IMHO your 'by' option above reads slightly better than the others, and highlights that rbenv is picked up on each host. They all make sense though :).

mattbrictson commented 7 years ago

Looks good! Thanks for contribution ✨