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

Update validation message if rbenv_ruby is not set #70

Merged

Conversation

ivanovaleksey
Copy link
Contributor

@ivanovaleksey ivanovaleksey commented Apr 6, 2017

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
Copy link

capistrano-bot commented Apr 6, 2017

1 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 🚫 Danger

@mattbrictson
Copy link
Member

@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 ivanovaleksey force-pushed the update-validation-message branch from bc64d8f to 42c28ba Compare April 7, 2017 05:40
@ivanovaleksey
Copy link
Contributor Author

ivanovaleksey commented Apr 7, 2017

I have updated the message a little bit:

  • use info instead of warn
  • use remote hosts instead of deployed hosts

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

@spikeheap
Copy link
Contributor

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
Copy link
Contributor Author

ivanovaleksey commented Apr 7, 2017

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

  • rbenv: rbenv_ruby is not set; ruby version will be defined by the remote hosts via rbenv
  • rbenv: rbenv_ruby is not set; ruby version will be defined on the remote hosts via rbenv

@spikeheap
Copy link
Contributor

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 :).

@ivanovaleksey ivanovaleksey force-pushed the update-validation-message branch from 42c28ba to 46ffb9f Compare April 7, 2017 12:41
@mattbrictson
Copy link
Member

Looks good! Thanks for contribution ✨

@mattbrictson mattbrictson merged commit 557ff98 into capistrano:master Apr 7, 2017
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.

4 participants