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

feat(intro-spector): recursively search for TODO items #1490

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benlubas
Copy link
Contributor

Before:
image

After:
image

There is now the weird behavior with nested lists that I point out in the example above. I still think this is an improvement, but feedback is of course welcome.

@max397574
Copy link
Contributor

do we even want this?
because if you have a normal list (with nested items or not) it will just show you how many of these items are completed
but now afaict it will e.g. for headings show how many nested things (e.g. todo list in the headings) are completed

@vhyrro
Copy link
Member

vhyrro commented Jun 28, 2024

I'm personally opposed to this (and hence why the implementation is as it stands today) - there's too much semantic fuzziness when you start mixing all TODO states with each other. This will be even more true once the spec is revised to allow lists to have more complex content inside. For instance, does an undone definition contribute to the state of a list item? If I have several TODO lists in a heading, do I really want to see [0/928] at the top?

I find this to make sense for the simple case, but for larger notes you'd start to see a thicket of introspector hints fairly quickly. Let's keep the behaviour as it is currently :)

@benlubas
Copy link
Contributor Author

Can we give it a configurable depth?I like when a heading has [n/x] for all its TODO items.

Default to 1 so it behaves the same as it does now. But allow people to set it how they want?

@vhyrro
Copy link
Member

vhyrro commented Jun 28, 2024

The depth is essentially always 1 currently, as the introspector looks for immediate children of the same type (e.g. all child headings) to get its information. This happens recursively, breadth-first iirc, so the whole thing becomes an inverted, tree-like fold.

An option could be to have a flag that allows for looking deeper and without these checks, but doing so feels off to me. I'd rather keep the existing behaviour. I feel like we underutilize the module system's capabilities of hotswapping modules. Perhaps it would just be wiser to create an external module with more lenient behaviour that hotswaps this one?

@Anrock
Copy link

Anrock commented Jun 28, 2024

For instance, does an undone definition contribute to the state of a list item? If I have several TODO lists in a heading, do I really want to see [0/928] at the top?

TBH sounds useful for my workflow. I often write big docs/specifications which are gradually refined and expanded with time and seeing which tree still has todo marks at a glance on top level is useful to see overall state of I-don't-know-ingness of current doc and what next experiment should be about.

@max397574
Copy link
Contributor

TBH sounds useful for my workflow. I often write big docs/specifications which are gradually refined and expanded with time and seeing which tree still has todo marks at a glance on top level is useful to see overall state of I-don't-know-ingness of current doc and what next experiment should be about.

but for that it doesn't need to be recursive
it's enough if it's not 100% imo
and as vhyrro pointed out this could perhaps be implemented externally because imo it doesn't make sense like this for most useres

@benlubas
Copy link
Contributor Author

Perhaps it would just be wiser to create an external module with more lenient behaviour that hotswaps this one

Could do this but it would just be needlessly duplicating code I think. If that's what you'd prefer I can do that

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.

4 participants