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

extensions refactor POC #46

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

Conversation

benjamonnguyen
Copy link

@benjamonnguyen benjamonnguyen commented Aug 22, 2023

This is my proposal to refactor the extension system (and tags as a whole):

I've found this way of handling and working with extensions to be a much easier developer experience that maintains the desired flexibility with the extension system.

It also resolves issue 45.

Rather than tracking spans, the body is treated as a space delimited string array and an array of indices is maintained for each tag to replace the current tracking logic.

If you'd like to pursue this implementation, I can implement the same pattern for projects/contexts and add tests.

@netlify
Copy link

netlify bot commented Aug 22, 2023

Deploy Preview for jstodotxt failed.

Name Link
🔨 Latest commit 6e4869f
🔍 Latest deploy log https://app.netlify.com/sites/jstodotxt/deploys/64e507f606bcf400084ba283

@ransome1
Copy link

Hi @benjamonnguyen . I havn't looked into your proposal here I can't really tell, what it is doing. But over at sleek (https://github.com/ransome1/sleek) I have some trouble implementing individual extensions, since the switch to the next branch of jstodotxt.

We have a use case in sleek, where we need to detect speaking dates for Due and Threshold.

Currently jstodotxt will detect due:2023-01-01 or t:2023-01-01 out of the box. But we need to use Sugar to also detect something like due:next week or t:tomorrow in 3 weeks.

In the 0.10.0 of jstodotxt, this was no problem, we would just write an custom extension for it.

Is you proposal here capable of tackling a use case like this one?

@benjamonnguyen
Copy link
Author

benjamonnguyen commented Aug 26, 2023

@ransome1 The todo.txt spec doesn't support whitespace for extensions.

https://github.com/todotxt/todo.txt#additional-file-format-definitions

Both key and value must consist of non-whitespace characters, which are not colons.

I'd recommend going with something like TxDx's solution.

If you were to pursue this format, maybe you can create a class that extends the jstodotxt/Item class and override the setBody() method to add your custom parsing logic.

This PR is trying to address the way tag positions are tracked using spans which in my experience seems to be more error prone and complicated.

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.

removeExtension() not working if no other text in body
2 participants