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

Default to raising exception if Rails app cannot be loaded #1523

Merged
merged 3 commits into from
Jun 13, 2023

Conversation

KaanOzkan
Copy link
Contributor

@KaanOzkan KaanOzkan commented May 31, 2023

Resolves #1500

Apps that want the previous functionality can use --no-halt-upon-load-error

Motivation

Current behaviour is hiding legitimate errors and requires inspecting the logs

Implementation

Raise the exception thrown from config/application.rb if the new halt-upon-load-error flag is set

Tests

Included.

option :halt_upon_load_error,
type: :boolean,
desc: "Halt upon a load error while loading the Rails application",
default: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we really need an option for this.

Do we really want to generate empty RBIs if the application didn't load?

Copy link
Contributor Author

@KaanOzkan KaanOzkan May 31, 2023

Choose a reason for hiding this comment

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

Good question. I think originally we defaulted to rescuing the exceptions to support "edge case" apps that were unable to boot config/application.rb but still want RBI generation. For DSLs most of the compilers do depend on Rails but not necessarily all. An app could be in a strange state but they can still want FrozenRecord compiler to generate some RBIs. Without a rescue, gem loading would also fail.

Do we really want to generate empty RBIs if the application didn't load?

Due to non-rails compilers we would be generating some RBIs in tapioca dsl so I can see the usecase.

I couldn't find any user reports but I feel like the use case above was an ask. This is where it was added as a safe_require so maybe @paracycle has more context.

Copy link
Member

Choose a reason for hiding this comment

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

I think we still need a flag, since users might not be able to "fix" the problem with their application being loaded, and if that is the case, then Tapioca is unable to generate anything at all. It could still be valuable for Tapioca to continue generating something in those cases, rather than nothing.

@KaanOzkan KaanOzkan marked this pull request as ready for review June 1, 2023 18:23
@KaanOzkan KaanOzkan requested a review from a team as a code owner June 1, 2023 18:23
@KaanOzkan KaanOzkan added the enhancement New feature or request label Jun 1, 2023
Copy link
Contributor

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

Nice! I have one question about naming -- halt-upon-load-error is descriptive but long, which might make it hard to remember. In rspec, a similar option is called fail-fast, which, imo, is super easy to remember. I'm not sure what other CLIs call this kind of option -- does the halt-upon-load-error option name come from somewhere, or conform to some already-established pattern? I'm not opposed to it, just curious if there's a more succinct option 😆

@KaanOzkan
Copy link
Contributor Author

Nice! I have one question about naming -- halt-upon-load-error is descriptive but long, which might make it hard to remember. In rspec, a similar option is called fail-fast, which, imo, is super easy to remember. I'm not sure what other CLIs call this kind of option -- does the halt-upon-load-error option name come from somewhere, or conform to some already-established pattern? I'm not opposed to it, just curious if there's a more succinct option 😆

I'm definitely open to naming suggestions. Reason I went with this one is because the flag is only used in specific load error instance. It's indeed long but it should be used once in the config file instead of in CLI during development. If we don't think there isn't enough benefit in being explicit I'm okay with something like fail-fast 👍

@egiurleo
Copy link
Contributor

I'm okay with a more descriptive flag that's only meant to be used in the config 👍🏻

@KaanOzkan KaanOzkan requested a review from Morriar June 12, 2023 21:14
@KaanOzkan KaanOzkan merged commit 34b31e0 into main Jun 13, 2023
30 checks passed
@KaanOzkan KaanOzkan deleted the ko/default-raise branch June 13, 2023 14:55
@shopify-shipit shopify-shipit bot temporarily deployed to production June 23, 2023 18:32 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default to raising exceptions if Rails app could not be loaded
4 participants