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

Fix for engine routes (2) #44

Closed
wants to merge 1 commit into from
Closed

Fix for engine routes (2) #44

wants to merge 1 commit into from

Conversation

blocknotes
Copy link
Contributor

@blocknotes blocknotes commented Mar 26, 2022

Hey @k0kubun.
I still have issues with an application with Rails engines... but it's tricky to create a spec for the case 😑

That's what I see:

  • with the current master version (0.47) I get again "No route matched for ..." exception;
  • applying the PR change to find_rails_route (but keeping the raise line) it gets better, but it still fails when there's a test on a non-existing path;
  • replacing the raise with the warn seems to solve the issue completely.

I think that the iteration of the routes with the current code still returns nil in some situations (probably the line return find_rails_route(request, app: route.app.app, fix_path: false)).

Think about it, if you don't feel confident to merge a PR without a good test it's ok (in any case having an alternative here could help someone else perhaps).


Relates to #38 and #37

@blocknotes blocknotes marked this pull request as ready for review March 26, 2022 08:29
@k0kubun
Copy link
Collaborator

k0kubun commented Mar 26, 2022

If you can't create a test case, please at least share a repository that reproduces it. You should at least have that one somewhere, and I think you can carve out what's still reproducing your issue to a small repository. Think about it, if I have no way to test my own code, it will randomly reproduce the same issue on your project again. So merging it temporarily doesn't mean anything if I can't reproduce it.

@blocknotes
Copy link
Contributor Author

If you can't create a test case, please at least share a repository that reproduces it. You should at least have that one somewhere, and I think you can carve out what's still reproducing your issue to a small repository. Think about it, if I have no way to test my own code, it will randomly reproduce the same issue on your project again. So merging it temporarily doesn't mean anything if I can't reproduce it.

I tried to isolate the behaviour without success and the project is closed source.
It's fine anyway - I'll keep the commit in my fork.

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.

2 participants