8000 Format checkboxes by rscharfe · Pull Request #1128 · gitgitgadget/gitgitgadget · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Format checkboxes #1128

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 20, 2022
Merged

Format checkboxes #1128

merged 1 commit into from
Oct 20, 2022

Conversation

rscharfe
Copy link
Contributor

GitHub task lists are represented in HTML as unordered lists with checkboxes. These checkboxes are currently ignored when converting the HTML to text. This affects PRs with task lists (e.g. git/git#1359); the resulting text list has no indication anymore whether an item was checked (done) or not.

E.g. this:

  • done item
  • item still to do

Is turned into:

* done item
* item still to do

Format checkboxes by turning checked ones into "[x]" and unchecked ones into "[ ]" to retain that information during the conversion. This turns the text result for the example above into:

* [x] done item
* [ ] item still to do

Unfortunately the simple formatter callback requires two ESLint annotations to avoid a warning and an error because it doesn't use all of its parameters and needs to access elem.attribs, which is untyped for some reason. Hopefully there's a better way, but that's the best I can do for now.

@dscho
Copy link
Member
dscho commented Oct 17, 2022

@rscharfe I force-pushed your branch, please 8000 review.

Range-diff

  • 1: e5b1da3 ! 1: df287d1 Format checkboxes

    @@
  ## Metadata ##
-Author: rscharfe <l.s.r@web.de>
+Author: René Scharfe <l.s.r@web.de>
 
  ## Commit message ##
     Format checkboxes
@@ Commit message
     for some reason.  Hopefully there's a better way, but that's the best I
     can do for now.
 
+    Test-added-by: Johannes Schindelin <johannes.schindelin@gmx.de>
+    Signed-off-by: René Scharfe <l.s.r@web.de>
+    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
+
  ## lib/markdown-renderer.ts ##
 @@ lib/markdown-renderer.ts: export function md2text(markdown: string, columns = 76): string {
                          .replace(/(^|\n)(\n)(?!$)/g, "$1>$2"); // quote empty
                      }});
              },
-+            // eslint-disable-next-line @typescript-eslint/no-unused-vars
-+            checkBoxFormatter: (elem, _walk, builder, _options) => {
-+                // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-+                builder.addInline(elem.attribs.checked === undefined ? "[ ]" : "[x]");
++            checkBoxFormatter: (elem, _walk, builder) => {
++                const attribs = elem.attribs as { checked?: boolean };
++                builder.addInline(attribs.checked === undefined ? "[ ]" : "[x]");
 +            },
          },
          selectors: [
@@ lib/markdown-renderer.ts: export function md2text(markdown: string, columns = 76
          ],
      };
  
+
+ ## tests/markdown-renderer.test.ts ##
+@@ tests/markdown-renderer.test.ts: const bq7 = `Over 56 levels still has 20 char (exact):
+ >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3 5 7 9 1 3 5 7 9 1
+ >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3 5 7`);
+ });
++
++test("task lists are rendered correctly", () => {
++    const taskList = `This is a task list:
++* [x] done item
++* [ ] item still to do`;
++    expect(md2text(taskList)).toEqual(`This is a task list:
++
++ * [x] done item
++ * [ ] item still to do`);
++});
+ \ No newline at end of file

rscharfe
rscharfe commented Oct 17, 2022
View reviewed changes
Copy link
Contributor Author
@rscharfe rscharfe left a comment

Choose a reason for hiding this comment

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

The paragraph lamenting the ESLint annotations should be replaced by a note saying that you fixed them, Dscho.

I give two more line-specific comments, hopefully just demonstrating lack of experience with JavaScript and TypeScript.

Thank you for the test!

lib/markdown-renderer.ts
@@ -36,6 +36,10 @@ export function md2text(markdown: string, columns = 76): string {
.replace(/(^|\n)(\n)(?!$)/g, "$1>$2"); // quote empty
}});
},
checkBoxFormatter: (elem, _walk, builder) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simply removing the unused function parameter didn't occur to me because using a different signature between caller and callee seems weird. I don't know JavaScript enough to know whether it can cause trouble.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if you are used to C it looks terrifying to "just drop" a parameter. Yet that's totally normal in Javascript.

lib/markdown-renderer.ts Outdated
@@ -36,6 +36,10 @@ export function md2text(markdown: string, columns = 76): string {
.replace(/(^|\n)(\n)(?!$)/g, "$1>$2"); // quote empty
}});
},
checkBoxFormatter: (elem, _walk, builder) => {
const attribs = elem.attribs as { checked?: boolean };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cast looks tricky -- hopefully just to me, lacking TypeScript experience. It would be nice if the type of elem could be inferred from the caller of this callback, but that's probably a bridge too far. elem.attrib also has a property named "type" (not optional, it seems), but we can simply ignore it here, I guess? https://github.com/html-to-text/node-html-to-text/blob/ba8697718d9995efbfe6255fe27d629e9d556b0b/lib/formatter.js#L147 has "const attribs = elem.attribs || {};"; would that work as well?

Copy link
Member

Choose a reason for hiding this comment

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

This cast basically says that we know that this any type is actually an object that has an optional checked attribute.

However, there is a bug: the attribute has type string ;-)

Will fix.

@rscharfe @dscho
Format checkboxes
dca8bc6 
GitHub task lists are represented in HTML as unordered lists with
checkboxes.  These checkboxes are currently ignored when converting the
HTML to text.  This affects PRs with task lists (e.g.
git/git#1359); the resulting text list has no
indication anymore whether an item was checked (done) or not.

Format checkboxes by turning checked ones into "[x]" and unchecked
ones into "[ ]" to retain that information during the conversion.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
dscho approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member
@dscho dscho left a comment

Choose a reason for hiding this comment

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

@rscharfe please have another look. As soon as you give the thumbs-up, I'll merge.

rscharfe
rscharfe commented Oct 18, 2022
View reviewed changes
Copy link
Contributor Author
@rscharfe rscharfe left a comment

Choose a reason for hiding this comment

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

Looks good to me! Indeed, checked is either absent or a string.

@dscho dscho merged commit d9e8091 into gitgitgadget:main Oct 20, 2022
@rscharfe rscharfe mentioned this pull request Nov 30, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dscho dscho dscho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rscharfe @dscho
0