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

Markdown #1328

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Conversation

benjamin-kirkbride
Copy link
Contributor

@benjamin-kirkbride benjamin-kirkbride commented Jul 3, 2023

Accidentally closed #1314 😅

Features:

  • indenting and dedenting of list items
  • pressing enter with a list will continue list at current indentation level
    • support task items
  • pressing enter on an empty list item will delete list item
  • continue task lists
  • continue > quotes

Closes #1305

@Akuli
Copy link
Owner

Akuli commented Jul 4, 2023

As for git messes: it's totally ok to make a mess with git! I use "squash and merge" in my projects, which basically means that a messy PR will appear as just one tidy commit when someone looks at the Git history.

You might also like https://akuli.github.io/git-guide/

Copy link
Owner

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

I like it overall :)

porcupine/plugins/markdown.py Show resolved Hide resolved
@@ -198,28 +198,6 @@ def check(filename, input_commands, output):
return check


def test_markdown_autoindent(check_autoindents):
check_autoindents(
Copy link
Owner

Choose a reason for hiding this comment

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

Can we keep this, but just modify it so that it passes?

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 got rid of it because it felt redundant with all the markdown tests, but I can look back and see if there is a way to make that happen

IS_LIST_ITEM_CASES.extend(
[
ListItemCase(id=f"numbered {i}", line=f"{i}{sep} item 1", expected=True)
for i, sep in itertools.product(itertools.chain(range(100), "#"), (".", ")"))
Copy link
Owner

Choose a reason for hiding this comment

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

These are maybe a bit too thorough :) We really don't need more than a thousand green dots when running pytest because of one regex. But more importantly, these two lines aren't very readable. It took me a while to figure out what they do.

Maybe instead add a few specific cases, so that it's easier to understand?

ListItemCase(id=f"numbered 1", line=f"1. item 1", expected=True)
ListItemCase(id=f"numbered 1", line=f"1) item 1", expected=True)
ListItemCase(id=f"numbered 4", line=f"4. item 1", expected=True)
ListItemCase(id=f"numbered 4", line=f"4) item 1", expected=True)
ListItemCase(id=f"numbered 42", line=f"42. item 1", expected=True)
ListItemCase(id=f"numbered 42", line=f"42) item 1", expected=True)
ListItemCase(id=f"numbered #", line=f"#. item 1", expected=True)
ListItemCase(id=f"numbered #", line=f"#) item 1", expected=True)

Also applies to other .extend()s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We really don't need more than a thousand green dots when running pytest because of one regex

But it's so fun 😩

these two lines aren't very readable

That's fair. It felt readable at the time 😓

Maybe instead add a few specific cases, so that it's easier to understand?

Yeah I can do this.

On a more broad note, how would you feel about adding https://hypothesis.readthedocs.io/en/latest/ to the test suite?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a big fan of hypothesis. IMO tests should be as concrete/specific as possible, such as ListItemCase(id="numbered 42", line="42) item 1", expected=True), whereas the code being tested tends to be written very generally. This ensures that at least the test is correct, because it's ideally very simple.

And yes, I know that the point of hypothesis is to catch bugs where a specific string that you didn't think of causes a problem. But they tend to get caught without unit tests in Porcupine. For example, the empty line.splitlines() bug caused errors when I opened a file.

I can see hypothesis being useful in other projects though. For example, maybe you have functions to escape and unescape a string in some way, and you want to know that they work in all corner cases.

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 would see hypothesis as purely additive, it wouldn't replace the simple ListItemCase(id="numbered 42", line="42) item 1", expected=True), sanity type tests, but it would replace:

IS_LIST_ITEM_CASES.extend(
    [
        ListItemCase(id=f"numbered {i}", line=f"{i}{sep} item 1", expected=True)
        for i, sep in itertools.product(itertools.chain(range(100), "#"), (".", ")"))

For example, the empty line.splitlines() bug caused errors when I opened a file.

I think there is likely many more of these sorts of things, especially as more and more plugins get added ;)

I think hypothesis would likely help a lot with the flaky tests as well. I should probably move this discussion to an issue.

@benjamin-kirkbride
Copy link
Contributor Author

@Akuli does the error in CI mean anything to you?

@Akuli
Copy link
Owner

Akuli commented Jul 7, 2023

The CI error has to do with #1300. Apparently the CI on Python 3.11 doesn't pass very reliably, even though you got it to pass most of the time in #1327.

I'd suggest we do nothing about the CI error if it disappears when tests run again. We will "soon" switch to pyright anyway.

@benjamin-kirkbride benjamin-kirkbride mentioned this pull request Jul 12, 2023
@benjamin-kirkbride
Copy link
Contributor Author

This PR is blocked by #1349

@benjamin-kirkbride
Copy link
Contributor Author

I am now going to work to finish this using Actions

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.

List Continuation for Markdown
3 participants