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

[ #60 , extract tasks ] #71

Merged
merged 6 commits into from Nov 27, 2023
Merged

[ #60 , extract tasks ] #71

merged 6 commits into from Nov 27, 2023

Conversation

mohamedsalem401
Copy link
Member

Issue #60 - Extract Tasks

Description:

Adding a new feature for extracting tasks from Markdown format strings.

Changes:

  • Uncompleted tasks. - [ ] task
  • Completed tasks. - [x] task and - [X] task
  • The tasks are added to the metadata of the files.

Copy link

changeset-bot bot commented Nov 25, 2023

🦋 Changeset detected

Latest commit: e1923b3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mddb Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@rufuspollock rufuspollock left a comment

Choose a reason for hiding this comment

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

I'm concerned that we extracting all list items as tasks which wouldn't be correct (unless that is our intention and we plan to have another field which indicates the list item is a task - like dataview does it)

MddbFileTag,
File,
} from "./schema.js";
import { MddbFile, MddbTag, MddbLink, MddbFileTag, File } from "./schema.js";
Copy link
Member

Choose a reason for hiding this comment

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

For me this "linting" stuff creates noise in the PRs. I strongly suggest we do this separately (it can just be a direct push to main if just linting).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing. The only thing that makes me lint on PR is our ci/cd complaining, but I will select the Github option to bypass this error.

image

Copy link
Member

Choose a reason for hiding this comment

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

@mohamedsalem401 👍

For future: we can lint the whole repo once and then we don't get running into these.

@@ -6,3 +6,9 @@ tags: tag1, tag2, tag3
# Welcome

[link](blog0.mdx)

- [] uncompleted task 1
Copy link
Member

Choose a reason for hiding this comment

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

Surely we need a pure list item ... which is not a task and check it is not showing up in our tasks list ...

We are in theory extracting tasks not just all list items

i.e. something like ...

- this is a list item
- this is another list item

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, of course, I tested for those before committing.

Actually, this exact line - [] uncompleted task 1 is a list item that's not being identified as a task because it's [] and not [ ], i.e., missing a space between the brackets, thus not a valid task (not valid in Obsidian, Github markdown, or even remark-parse).

Copy link
Member

Choose a reason for hiding this comment

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

@mohamedsalem401 ok that wasn't obvious because of the name. 😄

const nodes = selectAll("*", ast);
const tasks: Task[] = [];
nodes.map((node: any) => {
if (node.type === "listItem") {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like we will extract all list items as tasks which is not correct ....

if (node.type === "listItem") {
const description = recursivelyExtractText(node).trim();
const checked = node.checked;
if (checked !== null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@rufuspollock
You are right; we need to exclude listItems that are not tasks. That's why I am checking if checked is null to exclude the non-tasks list items, as per the documentation for GFM.
image

MddbFileTag,
File,
} from "./schema.js";
import { MddbFile, MddbTag, MddbLink, MddbFileTag, File } from "./schema.js";
Copy link
Member

Choose a reason for hiding this comment

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

@mohamedsalem401 👍

For future: we can lint the whole repo once and then we don't get running into these.

@rufuspollock rufuspollock merged commit 6a8a952 into main Nov 27, 2023
3 checks passed
@github-actions github-actions bot mentioned this pull request Nov 27, 2023
@mohamedsalem401 mohamedsalem401 deleted the tasks-extraction branch February 8, 2024 02:07
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