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

Enhance description lists #462

Merged
merged 3 commits into from
Nov 26, 2024
Merged

Enhance description lists #462

merged 3 commits into from
Nov 26, 2024

Conversation

digitalmoksha
Copy link
Collaborator

@digitalmoksha digitalmoksha commented Sep 2, 2024

Work in progress. Enhancing description lists.

  • Add support for multiple description definitions

    ```
    foo
    
    : bar
    : baz
    ```
    
  • Allow for the definition to follow after the term without a blank line

    ```
    foo
    : bar
    ```
    
  • Allow a definition list to be either "tight" or "loose", same as a normal list. Related to Description lists with tight and loose syntax #327

    Waffling on this one. "Tight" and "loose" lists add complexity, and is not well understood by users. Most implementations I've seen do support the distinction. However not supporting it means styling the list would be easier and less complex. 🤷

Copy link
Contributor

github-actions bot commented Sep 2, 2024

Command Mean [ms] Min [ms] Max [ms] Relative
./bench.sh ./comrak-235fbef 322.9 ± 1.7 319.5 325.8 2.94 ± 0.02
./bench.sh ./comrak-main 319.9 ± 1.4 317.1 323.6 2.91 ± 0.02
./bench.sh ./pulldown-cmark 109.8 ± 0.6 108.9 110.9 1.00
./bench.sh ./cmark-gfm 118.0 ± 0.8 116.5 119.8 1.08 ± 0.01
./bench.sh ./markdown-it 475.5 ± 1.3 472.4 477.9 4.33 ± 0.03

Run on Mon Sep 2 17:20:02 UTC 2024

Comment on lines 1856 to 2008
// ATTEMPT 1
// reopen_ast_nodes(last_child);
//
// let details =
// self.add_child(last_child, NodeValue::DescriptionDetails, self.first_nonspace + 1);
// *container = details;
//
// true

// ATTEMPT 2
// let parent = last_child.parent().unwrap();
// let item = parent.last_child().unwrap();
//
// reopen_ast_nodes(item);
//
// let details =
// self.add_child(item, NodeValue::DescriptionDetails, self.first_nonspace + 1);
// *container = details;
//
// true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kivikakk I was wondering if you could take a look at this.

I realized that when supporting multiple definitions, the second DescriptionDetails gets added as an entirely new DescriptionItem, rather than being added as a child to the existing DescriptionItem.

But both my attempts to do this resulted in the assert!(ast.open); in finalize_borrowed tripping.

Sample input

foo
: bar
: one

I thought maybe it's the shenanigans I'm doing at line 1760, the None case, but using the following input (with blanks lines) bypasses that and it still asserts.

foo

: bar

: one

Even with re-opening the ast node, it's not working. I'm missing some magic incantation. 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't get multiple detail nodes added to a description item. But thinking about it again, I really think it's ok - it just means that there can be multiple DescriptionItem with just a DescriptionDetails, but no term. Actually I think this is fine. The HTML formatter doesn't care.

@digitalmoksha digitalmoksha force-pushed the bw-description-lists branch 5 times, most recently from da3b3c5 to f4b0b9e Compare November 25, 2024 21:15
@digitalmoksha
Copy link
Collaborator Author

Hey @kivikakk I think this is done now. Running fuzzing, found a couple things initially, which are now fixed. But I'll let it run another few hours.

This has extended the syntax to allow for tight lists and multiple definition items per term, such as

Apple
: juice
: core

Orange
: fruit
: color

wdyt?

@kivikakk
Copy link
Owner

Hey @digitalmoksha, that sounds good to me — I'll have a proper read over of the PR tonight! Thank you for your patience with this!

Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

Bravo, this looks fantastic. Feel free to merge! 🙇‍♀️

@kivikakk kivikakk linked an issue Nov 26, 2024 that may be closed by this pull request
@digitalmoksha digitalmoksha merged commit dca0e13 into main Nov 26, 2024
38 checks passed
@digitalmoksha digitalmoksha deleted the bw-description-lists branch November 26, 2024 05:44
@kivikakk
Copy link
Owner

Would you like me to make a release?

@digitalmoksha
Copy link
Collaborator Author

Oh yeah, that would be awesome 🙇 🙇 🙇

@kivikakk
Copy link
Owner

Done! 🤍

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.

Description lists with tight and loose syntax
2 participants