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

Leave trailing slashes for urls if specified in permalink #313

Closed
wants to merge 1 commit into from
Closed

Leave trailing slashes for urls if specified in permalink #313

wants to merge 1 commit into from

Conversation

ChristianRiesen
Copy link
Contributor

No description provided.

@WyriHaximus
Copy link
Member

Hey @ChristianRiesen sorry for the delay in response and thank you for your ping on Twitter. Had a chat with @simensen earlier today about permalinks and his main worry was that this would mess with permalinks in some way causing issues with services that require those like Disqus. The tests you've added should cover that 👍 . I'll run some tests over the weekend and get back with any feedback/change requests/merge this PR depending on my findings. This is definitely something we want to clear any confusion with our users regarding this subject.

Could you also open a PR to our documentation to document this behavior: https://github.com/sculpin/sculpin.io/tree/master/source/documentation

@WyriHaximus
Copy link
Member

@ChristianRiesen looks good, I'll have a chat with @simensen again mainly because there is a part educating users and more maybe unforseen issues I have to look into.

In my case I change my perma link from :year/:month/:filename/ to :year/:month/:filename to get the same affect as without your PR. But for some reason my disqus URL still has a trailing /.

@ChristianRiesen
Copy link
Contributor Author

@WyriHaximus Added the pull request for the docs: sculpin/sculpin.io#102
Sorry, been a bit busy, it's nanowrimo season :)

@Xerkus Xerkus closed this in b4e368e Mar 13, 2017
Xerkus added a commit that referenced this pull request Mar 13, 2017
Xerkus added a commit that referenced this pull request Mar 13, 2017
@ChristianRiesen
Copy link
Contributor Author

Thank you for adding this fix, now I can switch back to the official version of sculpin again :)

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

2 participants