Replace Instances of Thor::Error with Tapioca::Error

[drewhoffer]

trafficstars

Motivation

This PR addresses #2208 which suggested replacing Thor::Error with a custom Tapioca::Error.

Implementation

• I feel my implementation may be naive in thinking that I would simply need to rename Thor::Errors to Tapioca::Error and am wondering if instead exit_on_failure? should be replaced or defined elsewhere.

  • I’m still getting familiar with Tapioca and Thor and I’m not yet confident that my approach fully captures what’s intended around the replacement of exit_on_failure?. I’d really appreciate any feedback or pointers on what is expected here.

• Replaced usage of Thor::Error with Tapioca::Error (which was already defined) across codebase.

• In CLI mode we should be exiting (used exit_on_failure?)

• In addon_mode we should be silently terminating RBI Generation (again using exit_on_failure?)

• Updated typo in comment going through some of the protobuf spec

Tests

• I haven’t added any tests yet. If tests are expected for this change, I’d be happy to add them. I would just need a hand with telling me where they should go. I'm still getting familiar with the codebase and would appreciate any help.

[KaanOzkan]

There are some CI failures, also I think the Thor::Error in lib/tapioca/helpers/config_helper.rb can be migrated too.

[drewhoffer]

@KaanOzkan I've caught some other edge cases manually testing here. When running an expected (but invalid) call such as: bundle exec tapioca annotations --no-netrc --netrc-file we get the desired output(red and bold):

> bundle exec tapioca annotations --no-netrc --netrc-file
Options `--no-netrc` and `--netrc-file` can't be used together

By adding the following to exe/tapioca

begin
  Tapioca::Cli.start(ARGV)
rescue Tapioca::Error => e
  puts e.message
  exit(1)
end

However, when having Thor handle the call, and before we get to our code, we get the following (note the invalid arguments):

> bundle exec tapioca gem --abc
Unknown switches "--abc"
Deprecation warning: Thor exit with status 0 on errors. To keep this behavior, you must define `exit_on_failure?` in `Tapioca::Cli`
You can silence deprecations warning by setting the environment variable THOR_SILENCE_DEPRECATION.

Do you have any suggestions on how we should handle this?

Reintroducing exit_on_failure? would technically go against the recent cleanup, but it ensures consistent behavior (correct exit codes, no Thor deprecation warnings) and aligns with the direction of using Tapioca::Error over Thor::Error.

happy to adjust based on what direction you prefer, just wanted to flag this edge case and see what you'd recommend.

[KaanOzkan]

@drewhoffer great catch. I think you're right. Let's fix the deprecation error by keeping exit_on_failure. Now we could remove the !@addon_mode check from exit_on_failure but I think it's better to be cautious and leave it as is.

I think this PR is good to go, was there anything else on your mind? I'll do a final manual test afterwards.

[drewhoffer]

If you'd like I can comment the method and just have a note that it was left for this reason and that further investigation would need to be done regarding how to remove it as a dependency? Otherwise I think it's good to go.

Let me know if you need anything else from my end (screenshots showing behaviour etc. to make your life easier)

Thanks again for the help/patience on this, was a blast to work on/learn from.

[KaanOzkan]

If you'd like I can comment the method and just have a note that it was left for this reason and that further investigation would need to be done regarding how to remove it as a dependency?

I think it's fine, comment can get outdated, we can rely on the Thor's warning in case we decide to remove it.

Thanks for your contribution 🙂