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 routing problem #36

Merged
merged 4 commits into from May 5, 2021
Merged

Fix routing problem #36

merged 4 commits into from May 5, 2021

Conversation

yen3
Copy link

@yen3 yen3 commented Feb 9, 2021

Original issue: dmontagu#154

Credit: WouldYouKindly https://github.com/WouldYouKindly

If we use class-based routing, the cbv causes the double routes.
WouldYouKindly solves the problem, but he does not the patch to the
repo. I really need the patch for my daily development so I copy his
solution to resolve.

If it causes anyone feels uncomfortable, I will delete my PR.

Sorry for the copy

Original Issue: dmontagu#154
Credit: WouldYouKindly <https://github.com/WouldYouKindly>

If we use class-based routing, the cbv causes the double routes.
WouldYouKindly solves the problem, but he does not the patch to the
repo. I really need the patch for my daily development so I copy his
solution to resolve.

If it causes anyone feels uncomfortable, I will delete my PR.

Sorry for the copy
@yairsimantov20
Copy link

is there any update for that pr?
would love to use that fix

@tnte
Copy link

tnte commented Mar 17, 2021

Looking forward to this PR being accepted. Having the same issue.

@yuval9313
Copy link
Owner

It doesn't passes tests, so I first need to figure whats going on there,
If everything is fine i'll update

@zon7
Copy link

zon7 commented Apr 22, 2021

I think the first test failing should be /items? instead of /items/?

@Caindrac
Copy link

Any news on getting these tests fixed?

@yuval9313
Copy link
Owner

yuval9313 commented May 5, 2021

I was lacking the update, The new code create an issue where @router.get as decorator doesn't works which means:

  1. The code was not forked properly
  2. The new changes breaks this functionality

I can try to fix the fork so it won't drop the multiple routes functionality but I'm lacking the time so I don't know how much time it is going to take, anyway I'm starting it now

EDIT:
I had a look in the code and I can describe what is causing the error
by doing as WouldYouKindly PR suggesting, I am removing the unbounded router therefore the self parameter becomes required again
I think I can have it fixed by deducting the prefix from the routes pre-adding them but I'm not sure I really like this fix because it involves string manipulation which is both risky and not very easy to read (sometimes)

@yuval9313 yuval9313 merged commit b6fcb77 into yuval9313:master May 5, 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.

None yet

6 participants