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

More friendly error message for nonexistent env #595

Merged
merged 4 commits into from
May 16, 2020
Merged

More friendly error message for nonexistent env #595

merged 4 commits into from
May 16, 2020

Conversation

korkey128k
Copy link
Contributor

@korkey128k korkey128k commented Apr 22, 2020

Example of new error message

Using movefile from spec/fixtures/Movefile

Error message before:

$ bin/wordmove pull -d -s -e staging
gems/wordmove/lib/wordmove/deployer/base.rb:16:in `deployer_for': undefined method `[]' for nil:NilClass (NoMethodError)

Error message after:

$ bin/wordmove pull -d -s -e staging
gems/wordmove/lib/wordmove/movefile.rb:59:in `environment': No environment found for "staging". Available Environments: remote (Wordmove::UndefinedEnvironment)

@alessandro-fazzi
Copy link
Member

Hey @korkey128k ! Thank you very much for you PR! It sounds really interesting.

I'd like to ask you two things:

  1. You should update the description of the PR, explaining at least the effect it has with an example. Would be great to see the error message before and after, just to be able to understand the value w/o having to reproduce the behaviour by myself
  2. what's your experience w/ ruby? I ask because test suite is red now and I don't know if I could help you with advises or directly with an intervention on the code. Knowing how much you're comfortable with the language will help me to help you ;)

Thanks so much in advance

@alessandro-fazzi alessandro-fazzi linked an issue Apr 22, 2020 that may be closed by this pull request
Copy link
Member

@alessandro-fazzi alessandro-fazzi left a comment

Choose a reason for hiding this comment

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

I'd like to ask you two things:

  • You should update the description of the PR, explaining at least the effect it has with an example. Would be great to see the error message before and after, just to be able to understand the value w/o having to reproduce the behaviour by myself
  • what's your experience w/ ruby? I ask because test suite is red now and I don't know if I could help you with advises or directly with an intervention on the code. Knowing how much you're comfortable with the language will help me to help you ;)

@korkey128k
Copy link
Contributor Author

Hey there,

  • I'll work on a better description. I also want to move the exception to shorten the method length (as per rubocop). As far as the error in Travis, I'm not experiencing that in my local version, I'll see what I can do to reproduce it.
  • I'm fairly comfortable in ruby, I believe I can fix the tests. I'll let you know if I need some assistance. 😀

Copy link
Member

@alessandro-fazzi alessandro-fazzi left a comment

Choose a reason for hiding this comment

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

Hey thanks for you iteration :)

This looks good to me 👍 Thanks a LOT

@alessandro-fazzi
Copy link
Member

Waiting to merge just because I'd like to merge and release together with other PRs :)

@alessandro-fazzi alessandro-fazzi merged commit 110dd6f into welaika:master May 16, 2020
alessandro-fazzi added a commit that referenced this pull request Nov 15, 2021
alessandro-fazzi added a commit that referenced this pull request Dec 26, 2021
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.

Better error message when the env doesn't exist
2 participants