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

Update routing.md #920

Merged
merged 5 commits into from
Oct 6, 2023
Merged

Update routing.md #920

merged 5 commits into from
Oct 6, 2023

Conversation

KalynDavis
Copy link
Contributor

Fixes typo on Basics docs section related to 'redirect(to:type:)' being deprecated and renamed to 'redirect(to:redirectType:)'

Fixes typo on docs related to 'redirect(to:type:)' being deprecated and renamed to 'redirect(to:redirectType:)'
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

@KalynDavis Thanks for this! Since this is just code changes could you update the different translations as well?

@@ -424,7 +424,7 @@ req.redirect(to: "/some/new/path")
You can also specify the type of redirect, for example to redirect a page permanently (so that your SEO is updated correctly) use:

```swift
req.redirect(to: "/some/new/path", type: .permanent)
req.redirect(to: "/some/new/path", redirectType: .permanent)
```

The different `RedirectType`s are:
Copy link
Member

Choose a reason for hiding this comment

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

This line needs to be updated as well

Suggested change
The different `RedirectType`s are:
The different `Redirect`s are:

Copy link
Contributor Author

@KalynDavis KalynDavis Oct 5, 2023

Choose a reason for hiding this comment

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

@0xTim shouldn't this still be RedirectType's since it is providing the different static members for redirectType?

I have updated the other translations for the original change.

Copy link
Member

Choose a reason for hiding this comment

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

@KalynDavis RedirectType is now deprecated and the new type is Redirect so we should rename it just to avoid confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xTim Oh, My apologies! I was under the impression that this was type->redirectType. To clarify, it is redirectType->redirect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked the source code. I see the changes. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xTim I've updated the translations to properly reflect the changes. Thanks for pointing that out!

Note: I did notice that the German translation doesn't look to have the same formatting as the other translations. Gives me the impression the information might be outdated as well.

Copy link
Member

Choose a reason for hiding this comment

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

The German translation is fine. 😊. Just different.

@KalynDavis KalynDavis requested a review from a team as a code owner October 5, 2023 21:53
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks!

@0xTim 0xTim enabled auto-merge (squash) October 6, 2023 11:56
@0xTim 0xTim merged commit fbd9723 into vapor:main Oct 6, 2023
1 check passed
@penny-for-vapor penny-for-vapor bot mentioned this pull request Oct 6, 2023
9 tasks
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.

3 participants