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

Makes most special characters be replaced with a dash #4783

Merged
merged 1 commit into from
Jun 4, 2015

Conversation

claydiffrient
Copy link
Contributor

closes #4782

  • Still achieves the same goal of stripping out reserved characters
  • Changes from removal to replacement
  • This helps keep word separators from being removed
  • Apostrophes (') are unaffected

@claydiffrient claydiffrient changed the title Makes @ be replaced with a dash in slugs Makes most special characters be replaced with a dash Jan 14, 2015
@ErisDS
Copy link
Member

ErisDS commented Mar 21, 2015

Hi @claydiffrient, sorry for letting this sit for such a long time. I think the order in which things are done needs to be addressed, as currently the £ handling is going to cause an issue with any title which contains the letters PS - you can see that this test will fail.

I recommend switching it back to doing character handling before unicode handling?

@ErisDS
Copy link
Member

ErisDS commented Apr 20, 2015

Hi @claydiffrient, if you don't have time to update this then I'll fix it up and get it merged. Thought I'd drop one more message before I do JIC 😉

@claydiffrient
Copy link
Contributor Author

Hey @ErisDS I'm so sorry that I've not done this yet. I keep putting it off. I'll fix it up tonight or tomorrow. If it's not done by Wednesday, I give you leave to take it :)

@ErisDS
Copy link
Member

ErisDS commented Apr 20, 2015

Understood 😀

@claydiffrient
Copy link
Contributor Author

@ErisDS I've got commits in that fix everything. I think the one failure on that one build is a transient failure. All the other builds worked just fine, and I wouldn't expect this code to affect that portion. I also cannot get it to fail locally. I can't retrigger the build on Travis for some reason, I'm assuming because I'm not a collaborator on the project. I think it would be safe to override and do the merge, but perhaps retriggering would be best to get the green check from Travis.

Sorry it took me so long to get this done.

@ErisDS
Copy link
Member

ErisDS commented Apr 22, 2015

Hey @claydiffrient I restarted that failing build and it's all green now :) One last thing, could you please squash those commits? Thanks 👍

@claydiffrient
Copy link
Contributor Author

@ErisDS Done. Sorry, I can't believe I didn't do that beforehand. I hate getting several commits in PRs personally.

@ErisDS
Copy link
Member

ErisDS commented Apr 27, 2015

I'm sorry for the to-ing and fro-ing on this, but I think the most recent change to account for £ has re-introduced other issues.

I believe the £ needs to be handled first and separately, then unidecode, and then the other character replacements. The reason being that unidecode happening after will result in some characters not being substituted correctly. If you turned the example from the unidecode readme into a test I think you'd see:

に間違いがないか、再度確認してください。再読み込みしてください。

gets converted to

niJian Wei iganaika, Zai Du Que Ren sitekudasai. Zai Du miIp misitekudasai.

Which introduces new commas, full stops, and a space at the end which would not be handled. We may even need to move the call to .trim() ?

@claydiffrient
Copy link
Contributor Author

@ErisDS I added the unicode test and made sure it passes. I also went ahead and moved the call to trim just to be safe. I couldn't figure out a test that would cause it to have problems running at the beginning, but figured maybe there was something that it would fail with somewhere out there. Anyways, hopefully this works in all cases now :)

@ErisDS
Copy link
Member

ErisDS commented May 13, 2015

@claydiffrient not sure if you saw but this PR is currently failing on a linting error :(

closes TryGhost#4782
- Still achieves the same goal of stripping out reserved characters
- Changes from removal to replacement
- This helps word separators from being removed
- Apostrophes (') are unaffected
@claydiffrient
Copy link
Contributor Author

@ErisDS I actually didn't notice. I fixed the problem it was hung up on and everything seems to be good locally. A new commit is up. Hopefully this will be the end of it :)

ErisDS added a commit that referenced this pull request Jun 4, 2015
Makes most special characters be replaced with a dash
@ErisDS ErisDS merged commit 5e0cc1a into TryGhost:master Jun 4, 2015
@ErisDS
Copy link
Member

ErisDS commented Jun 4, 2015

Merged! I actually thought I had done this already 😕

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.

Generated post URL from title should replace @ with dash
2 participants