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

Ensure a single leading @ for twitter usernames #367

Merged
merged 3 commits into from
Nov 13, 2019
Merged

Ensure a single leading @ for twitter usernames #367

merged 3 commits into from
Nov 13, 2019

Conversation

samtrion
Copy link
Contributor

This prevents destroying the template due to the incorrect use of quotes.

This prevents destroying the template due to the incorrect use of quotes.
@ashmaroli
Copy link
Member

Did you know?

Liquid also has a remove filter that seems to be more appropriate than replace in this use-case.

@samtrion
Copy link
Contributor Author

I did not know. However, I have taken the Replace from the previous version.

@ashmaroli
Copy link
Member

I understand that. But since the lines are being updated, its always best to use the most apt route.

Replaced `replace` with `remove`
@ashmaroli ashmaroli changed the title Update template.html Ensure a single leading @ for twitter usernames Oct 31, 2019
@samtrion
Copy link
Contributor Author

Done

@ashmaroli
Copy link
Member

Thanks for updating. Do you by any chance know if @ is allowed after the first char? (e.g. @zombie@)

@samtrion
Copy link
Contributor Author

Sorry, as far as I know the first @ is required, but if additional @ are allowed, I don't know

@samtrion
Copy link
Contributor Author

samtrion commented Oct 31, 2019

https://help.twitter.com/en/managing-your-account/twitter-username-rules

It's not allowed:

A username can only contain alphanumeric characters (letters A-Z, numbers 0-9) with the exception of underscores...

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

To avoid unnecessary String allocations.. (Certain Liquid filters duplicate the input string before processing it.)

@samtrion
Copy link
Contributor Author

samtrion commented Nov 1, 2019

Done

@ashmaroli ashmaroli requested a review from a team November 1, 2019 10:26
@parkr
Copy link
Member

parkr commented Nov 1, 2019

A test could be helpful so we don't regress in the future!

@DirtyF DirtyF added this to the Next minor version milestone Nov 13, 2019
@DirtyF
Copy link
Member

DirtyF commented Nov 13, 2019

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 16a5e3d into jekyll:master Nov 13, 2019
jekyllbot added a commit that referenced this pull request Nov 13, 2019
@jekyll jekyll locked and limited conversation to collaborators Nov 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants