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

added replace hashtags and remove hashtag #58

Merged
merged 3 commits into from
Jul 11, 2020

Conversation

ishanarora04
Copy link
Contributor

No description provided.

@ishanarora04
Copy link
Contributor Author

ishanarora04 commented Jul 10, 2020

Hey, @jbesomi Can you check the PR ?

@jbesomi
Copy link
Owner

jbesomi commented Jul 10, 2020

Hey @ishanarora04, it looks awesome!

Two very small improvements we can make:

  • As we write Example: #texthero_123., we can have the same hashtags example in the Example. I.e we can replace "Hi #hashtag, we will remove you." with "Hi #texthero_123, we will remove you.". Consistency is very useful when reading documentation.
  • See also: "for replacing a tag with a custom symbol." should be "for replacing a hashtag with a custom symbol."

(Edit) when opening a new PR, is a good practice to add a description of what you changed and why. This helps other developers understand quickly and better what you changed.

Regards,

@ishanarora04
Copy link
Contributor Author

Done. Will ensure a full fledged description going forward.

@jbesomi
Copy link
Owner

jbesomi commented Jul 10, 2020

Thank you! Why on the replace_tags the pattern had round brackets r"@([a-zA-Z0-9]+)" whereas in replace_hashtags there are not? "#[a-zA-Z0-9_]+".
Also, why you don't specify the r in the second case? Details matter
Can you please make a small research and argument your decisions? Sorry for being picky, just want to ensure we do not introduce bugs. Also, probably in the rest of the code we might have similar discrepancies. It would be great to fix all of this issues in case (in a separate PR).

By the way, how good is your knowledge of regular expression? If you need a recap, this is a super great website: https://www.regular-expressions.info/

Regards,

@ishanarora04
Copy link
Contributor Author

ishanarora04 commented Jul 11, 2020

r is used to ensure a given string is treated as raw string. https://stackoverflow.com/questions/33729045/what-does-an-r-represent-before-a-string-in-python. String literals may optionally be prefixed with a letter 'r' or 'R'; such strings are called raw strings and use different rules for interpreting backslash escape sequences. from (https://docs.python.org/2/reference/lexical_analysis.html#string-literals)

The brackets are for the capturing group. Both the forms will provide with the desired effect.
However for our purpose 2nd form is better. I can provide a small PR asap to fix it in the tag form.

Also refer to this: https://www.quora.com/Can-you-explain-what-this-regular-expression-r-means-in-Python#:~:text=Django%20URL%20configurations.-,The%20'r'%20in%20front%20tells%20Python%20the%20expression%20is%20a,escape%20sequences%20are%20not%20parsed.&text=an%20'n'.-,Raw%20strings%20are%20handy%20in%20regex%2C%20in%20which%20the%20backslash,often%20for%20its%20own%20purposes.

It is conventionally used.

@jbesomi
Copy link
Owner

jbesomi commented Jul 11, 2020

Perfect, thank you! 🎉🎉

Feedback: when you post links on a comment when possible make use of the markdown link tags, it will be easier for others to read and understand!

Looking forward to see your next PR, you are learning soo fast!

@jbesomi jbesomi closed this Jul 11, 2020
@jbesomi jbesomi reopened this Jul 11, 2020
@jbesomi jbesomi merged commit a9ce14c into jbesomi:master Jul 11, 2020
henrifroese pushed a commit to SummerOfCode-NoHate/texthero that referenced this pull request Jul 12, 2020
* added replace hashtags and remove hashtag

* Fixed the Documentation

* Preprocessing Hashtag Regex as a raw string
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.

2 participants