Skip to content

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Feb 12, 2021

Draft PR since it depends on the changes from #5139 (now merged)

@RasmusWL RasmusWL force-pushed the django-get-redirect-url branch from a582d86 to 7451484 Compare February 15, 2021 09:56
@RasmusWL RasmusWL marked this pull request as ready for review February 15, 2021 10:21
@RasmusWL RasmusWL requested a review from a team as a code owner February 15, 2021 10:21
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

Nice solution with getFirstPossibleRoutedParamIndex.

Comment on lines +2165 to +2167
* Note: this function only does something on a subclass of `RedirectView`, but since
* classes can be considered django view classes without us knowing their super-classes,
* we need to consider _any_ django view class. I don't expect any problems to come from this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I was wondering about this 👍

routeHandler.getArg(routeHandler.getRequestParamIndex() + regex.getGroupNumber(_, _))
routeHandler
.getArg(routeHandler.getFirstPossibleRoutedParamIndex() - 1 +
regex.getGroupNumber(_, _))
or
Copy link
Contributor

Choose a reason for hiding this comment

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

I might have written the -1 at the end because I think of it as shifting group numbers from 1-based to 0-based.

Copy link
Member Author

Choose a reason for hiding this comment

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

that would probably have made more sense 😆

@yoff yoff merged commit e3b3825 into github:main Feb 25, 2021
@RasmusWL RasmusWL deleted the django-get-redirect-url branch February 26, 2021 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants