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

Removing html elements from events description. #19

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

edenxcodes
Copy link

@edenxcodes edenxcodes commented Sep 26, 2023

Closes #16

Copy link
Member

@oliviasculley oliviasculley left a comment

Choose a reason for hiding this comment

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

I'm using the emoji code review standard, so ⛏️ represents comments that don't necessarily need to be changed and 🔧 needs to be changed. Since the only item that should be changed is relatively minor (the private function naming), i've gone ahead and approved this PR so it can be merged in once the changes have been made!

Thank you for contributing!!

Comment on lines +10 to +14
# import regular expression module
import re



Copy link
Member

Choose a reason for hiding this comment

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

⛏️ There isn't a big need for this import to be separate from the other imports, and I think the comment isn't totally necessary

Comment on lines +22 to +23


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

⛏️ These lines can be removed

Comment on lines +29 to +38
self.description = self.remove_html(description)
self.location = location
self.time = time
self.url = url
self.status = status
self.uuid = uuid

def remove_html(self, string):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.description = self.remove_html(description)
self.location = location
self.time = time
self.url = url
self.status = status
self.uuid = uuid
def remove_html(self, string):
self.description = self.__remove_html(description)
self.location = location
self.time = time
self.url = url
self.status = status
self.uuid = uuid
def __remove_html(self, string):

🔧 Since this function is only intended to be used in Event's __init__ constructor, we'll want to add two underscores in order to mark this as a private function that isn't intended to be used by other people. But otherwise, separating the HTML sanitation into a separate function is a great approach!

@oliviasculley
Copy link
Member

Oh also let me know your preferred information so we can add you to the list of contributors in the README!

@edenxcodes
Copy link
Author

Hi @oliviasculley, thanks a bunch for the feedbacks.
I don't have any preferred information. You can add whatever you think is best based on my contribution and a link back to my github. And what's your twitter handle? Let's connect. Thank you! xD

@oliviasculley
Copy link
Member

I don't have a twitter, but I'm active on the HackGreenville slack group at https://hackgreenville.com.

@all-contributors please add @edenxcodes for code !

@allcontributors
Copy link
Contributor

@oliviasculley

I've put up a pull request to add @edenxcodes! 🎉

@edenxcodes
Copy link
Author

Hi @oliviasculley, my profile isn't showing on the contributors list yet xD. Do I have to accept it or something? (pardon my ask, I'm new to open source contributions :D ).

@oliviasculley
Copy link
Member

We'll need to get this PR merged into dev first, and then we should be able to add the contributors PR after that. Will you be at the first Hackgreenville Nights this Thursday? We can do some pair programming to get this PR passing all the tests, it should be pretty quick so we won't need to miss any talks or anything

@ThorntonMatthewD
Copy link
Collaborator

Hello @edenxcodes! 😄 I wanted to check in to see if there was any way I could support you in helping to get this PR ready to rock. I think aside from the merge conflict it's pretty much ready to 🚢. I'm off work this Friday and have some availability if you'd like to collaborate at any point during the day. Just let me know and I'd be down to get something together!

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.

Sanitize event descriptions
3 participants