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

rework table richtext bug fix #95

Merged
merged 4 commits into from
Jan 18, 2023
Merged

rework table richtext bug fix #95

merged 4 commits into from
Jan 18, 2023

Conversation

sHermanGriffiths
Copy link
Contributor

@sHermanGriffiths sHermanGriffiths commented Dec 21, 2022

Author's Checklist

Before assigning reviewers, step through the following checklist. Check each item once it's completed. If an item is skipped, write an explanation below the item.

  • Construction: Review the code diff and confirm there's no dead code, debug code, or unnecessary comments.
  • Requirements: Review the requirements in the issue for this PR and confirm they're all implemented.
  • Architecture: Confirm that the changes to the code are consistent with the existing architecture.
  • SOUP/OTS Software: Any new dependencies have appropriate licenses and are documented or pinned in requirements files.
    N\A
  • Unit Test: Atomic unit tests have been added, if appropriate.
  • End-to-End Tests: End-to-end tests have been extended or updated, if appropriate.
    N\A
  • Low-level Documentation: Comments or doc strings for affected code have been updated.
    N\A
  • High-Level Documentation: The README is updated, as appropriate.
    N\A
  • Change Log: New features or bug fixes have been added to the change log in the README.
    N\A

Copy link
Member

@bimbashrestha bimbashrestha left a comment

Choose a reason for hiding this comment

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

Nice work @sHermanGriffiths!

Add a quick test for your change and this looks good to go. Maybe we can just edit the test case you deleted?

@@ -21,8 +21,3 @@ def test_plain_text_to_pandoc_spaces_after():
def test_plain_text_to_pandoc_spaces_after_newline():
pandoc_ast = RichText.plain_text_to_pandoc("hello\n world")
assert pandoc_ast == [Str("hello"), LineBreak(), Space(), Space(), Str("world")]


def test_plain_text_to_pandoc_table():
Copy link
Member

Choose a reason for hiding this comment

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

I assume you're deleting this test case because it breaks after your changes? Let's fix the test case so that it works with your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created it as a test case for the previous changes

@bimbashrestha
Copy link
Member

Thanks for pulling this out into it's own PR! Reviewing it like this is so much easier :)

Copy link
Member

@bimbashrestha bimbashrestha left a comment

Choose a reason for hiding this comment

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

@sHermanGriffiths please add a test that catches the bug you fixed in this PR before merging. Good job with the fix.

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.

None yet

2 participants