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

extract code, text as list #26

Closed
wants to merge 11 commits into from
Closed

extract code, text as list #26

wants to merge 11 commits into from

Conversation

ChillarAnand
Copy link
Contributor

only question, answer object need these methods.
this resolves #21

@Ffisegydd
Copy link
Member

Having User and Question, Answer, Comment inherit from the same object makes no sense as they're different things.

I propose we have a SEObject as the absolute base class, so everything inherits from this, and for now it can be empty as we don't have any methods that are needed by all (apart from, maybe, self._data = data). We then have something called Post that inherits from SEObject and that Question/Answer/Comment then inherit from Post. So Post would have the code based methods.

This would allow us some flexibility (in assigning stuff to SEObject if we needed to) as well as make sense for User not to have a get_code etc method.

@classmethod
def _get_text(cls, html):
soup = BeautifulSoup(html)
[s.extract() for s in soup('code')]
Copy link
Member

Choose a reason for hiding this comment

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

What does this line do? It's a list comprehension which isn't assigned to anything. I think I see what it does, that it extracts code tags so that they're not in soup anymore when you get the text, but can you confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. it extracts code elements. since we don't need code there, i have not stored it there.

@Ffisegydd
Copy link
Member

I need to do some more testing with this, to make sure it makes sense in my head and that it does what it is intended. It also needs more unittests to increase the coverage. But looking good :D

@Ffisegydd
Copy link
Member

I just did a quick test and I got text as one big long string, rather than a list of strings. I need to test more later but it doesn't look like it's working properly yet.

@Ffisegydd
Copy link
Member

Hey, do you have any idea who K3 is? They've pushed 3 commits, and I don't understand how or who they are.

super(Answer, self).__init__()

self._data = data
super(SEObject, self).__init__()
Copy link
Member

Choose a reason for hiding this comment

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

This should be super(Answer, self).__init__(). Same for the other classes.

@ChillarAnand
Copy link
Contributor Author

removed boiler plate code using super.

@@ -0,0 +1,30 @@
from ..objects import Post, Question, Answer, User, Comment

d = {'Body': '<p>bar</p><code>x=1</code>'}
Copy link
Member

Choose a reason for hiding this comment

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

Should d be defined in each test? I know that this increases the lines of code, but it also keeps each test independent of each other.

@Ffisegydd
Copy link
Member

(As discussed in chat) will check this out later tonight, check for PEP8, maybe add some more tests, then see about merging.

@Ffisegydd
Copy link
Member

Merged with 998a8bd

@Ffisegydd Ffisegydd closed this Nov 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants