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

Only use DidYouMean-integrated Error for Component loading failure #261

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

cllns
Copy link
Member

@cllns cllns commented Nov 15, 2022

Without this, any NameError (including NoMethodError) will go through the DidYouMean integration and
it will fail since that code expects only a Constant, not any name (including "undefined local variable or method")

Instead, this code will ensure we only try to do that when we get a NameError for specifically trying to load the constant we're looking for. Everything else will be re-raised and go through the normal flow (including any more DidYouMean default behavior).

Fixes hanami/hanami#1252. (Specifically see this comment)

Bug introduced in #217

Thoughts on whether we should test this? We could create a fixture file to load a component that has a NameError in it and ensure that error passes through instead of getting created as a ComponentNotLoadableError... but that would error out anyway. Creating a NameError in the spec just raises that before we can even get to the example.

I manually confirmed that it works with the repo @katafrakt created to illustrate the bug.

@cllns cllns requested a review from timriley November 15, 2022 23:12
@cllns cllns requested a review from solnic as a code owner November 15, 2022 23:12
@timriley
Copy link
Member

@cllns This sounds and looks good!

Do you reckon there's any chance you could add a test demonstrating one of these unrelated errors not getting caught up in our DidYouMean handling?

@timriley
Copy link
Member

timriley commented Nov 15, 2022

Oh, haha, I just read your description more closely.

We could create a fixture file to load a component that has a NameError in it and ensure that error passes through instead of getting created as a ComponentNotLoadableError

This sounds about right to me. I'd want this to be an integration-style test more than anything lower-level, since it's the end-to-end user experience we care about particularly here. Couldn't the test wrap the loading of that file in a before/rescure, or in an expect { }.to raise_error construct?

@cllns
Copy link
Member Author

cllns commented Nov 16, 2022

@timriley Can you take a look at my newest commit? I haven't been able to reproduce the issue from hanami/hanami#1252. Note that I've added || true to the main conditional here so it's effectively the existing library code. I want to reproduce the existing behavior so I can be sure this fix works as desired.

I've included comments in the spec/integration/did_you_mean_integration_spec.rb file. The first one, about finalize, isn't exactly necessary, just something that surprised me based on my (now rusty) memory of dry-system's terminology. The second one, after system["undefined_variable"] explains where the issue is. I guess the question is: what is Hanami doing that I'm not doing here... A lot! I guess, but what's relevant to the Loader not seemingly being used here?

Without this, *any* NameError (including NoMethodError)
will try to use the DidYouMean integration and
it will fail since that code expects only a Constant
@solnic solnic force-pushed the fix-did-you-mean-integration branch from 8bafd89 to 4aed4fe Compare November 18, 2022 07:30
@solnic solnic merged commit 798fe71 into main Nov 18, 2022
@solnic solnic deleted the fix-did-you-mean-integration branch November 18, 2022 07:32
@cllns
Copy link
Member Author

cllns commented Nov 18, 2022

Thanks for finishing the spec @solnic!

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.

Cryptic error when making a mistake in params definition
3 participants