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 support for lists #11

Closed
wants to merge 3 commits into from

Conversation

donaldwasserman
Copy link

Fixes #10

Copy link
Member

@n0str n0str left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request. Unfortunately, it has major bugs. It would be amazing if you fix them.

@@ -55,6 +57,13 @@ def handle_starttag(self, tag, attrs):
self.output += attr[1] + '|'
if tag == 'style' or tag == 'script':
self.skip = True
if tag == 'ol':
self.ol_counter_cache = 1
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this variable will never be incremented

self.ol_counter_cache = 1
if tag == 'li':
if self.ol_counter_cache is not None:
self.output += "%s." % self.ol_counter_cache
Copy link
Member

Choose a reason for hiding this comment

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

Please, use .format style:
"{}.".format(self.ol_counter_cache)

@@ -72,6 +81,8 @@ def handle_endtag(self, tag):
self.output += '`'
if tag == 'style' or tag == 'script':
self.skip = False
if tag == 'ol':
Copy link
Member

Choose a reason for hiding this comment

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

There is no ul tag.

Copy link
Author

Choose a reason for hiding this comment

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

based on the other code here, I'm not sure a ul is needed -- i'm not sure what it would do here.

test_general.py Outdated

def test_unordered_list():
html = '<ul><li>Hello</li></ul>'
expected = "- Hello"
Copy link
Member

Choose a reason for hiding this comment

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

It is necessary to append

    output = HTMLSlacker(html).get_output()
    assert (output == expected)

Otherwise the test will not work.

test_general.py Outdated

def test_ordered_list():
html = '<ol><li>one</li><li>two</li></ol>'
expected = "1. one \n 2. two"
Copy link
Member

Choose a reason for hiding this comment

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

Test is not passing

>       assert (output == expected)
E       AssertionError: assert '1.one1.two' == '1. one \n 2. two'
E         - 1.one1.two
E         + 1. one 
E         +  2. two

test_general.py:49: AssertionError

@donaldwasserman
Copy link
Author

@n0str Sorry - i left some unsaved changes in my editor.

Updated with your comments, but my only issue is I can't figure out how to get a \n in between the li elements. Any suggestions?

@Klikini
Copy link

Klikini commented Aug 17, 2022

Just curious, why was this closed before merging? Currently I have to preprocess the HTML and convert lists to lines starting with a unicode bullet character, which is pretty hacky, so this would be really nice to have.

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.

Add support for lists
3 participants