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: Add executeToggleTaskDoneCommand to Tasks Api #2781

Merged
merged 11 commits into from
May 8, 2024

Conversation

mgmeyers
Copy link
Contributor

@mgmeyers mgmeyers commented Apr 23, 2024

Description

This PR exposes the toggleLine function and the two task serializers through the API.

Motivation and Context

In combination with #2778, these API updates give the Kanban plugin everything it needs to add nearly full support for the task plugin.

How has this been tested?

Tests have been run. The API functionality has been validated through the latest Kanban beta.

Screenshots (if appropriate)

Screen.Recording.2024-04-23.at.11.29.24.AM.mov

Types of changes

Changes visible to users:

  • Bug fix (prefix: fix - non-breaking change which fixes an issue)
  • New feature (prefix: feat - non-breaking change which adds functionality)
  • Breaking change (prefix: feat!! or fix!! - fix or feature that would cause existing functionality to not work as expected)
  • Documentation (prefix: docs - improvements to any documentation content for users)
  • Sample vault (prefix: vault - improvements to the Tasks-Demo sample vault)
  • Contributing Guidelines (prefix: contrib - any improvements to documentation content for contributors - see Contributing to Tasks)

Internal changes:

  • Refactor (prefix: refactor - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
  • Tests (prefix: test - additions and improvements to unit tests and the smoke tests)
  • Infrastructure (prefix: chore - examples include GitHub Actions, issue templates)

Checklist

Terms

@claremacrae
Copy link
Collaborator

Hi, thanks for doing this.

I’m a bit confused by the checkboxes about documentation, but please do supply user documentation and tests for this. Many thanks.

@mgmeyers
Copy link
Contributor Author

Added!

@claremacrae
Copy link
Collaborator

I have really mixed feelings about this.

On the one hand, it's nice to see the code being reused.

On the other hand, there are ideas to rewrite the Tasks serializer code in the next few months, to make it more flexible... and exposing the current implementation means I will feel obliged to maintain support for the current code indefinitely.

@claremacrae
Copy link
Collaborator

On the other hand, there are ideas to rewrite the Tasks serializer code in the next few months, to make it more flexible... and exposing the current implementation means I will feel obliged to maintain support for the current code indefinitely.

The thinking is to use ideas from this design:

https://github.com/eatgrass/obsidian-tasks-deserializer

@mgmeyers
Copy link
Contributor Author

mgmeyers commented Apr 24, 2024

I have really mixed feelings about this.

On the one hand, it's nice to see the code being reused.

On the other hand, there are ideas to rewrite the Tasks serializer code in the next few months, to make it more flexible... and exposing the current implementation means I will feel obliged to maintain support for the current code indefinitely.

Totally fair. What if the serializer related API looked like this instead?

/**
 * Retrieves task details from a Tasks Emoji Formatted string
 *
 * @param line he markdown string of the task to be deserialized
 * @returns {TaskDetails}
 */
deserializeEmojiTaskLine(line: string): TaskDetails;

/**
 * Retrieves task details from a Dataview formatted string
 *
 * @param line he markdown string of the task to be deserialized
 * @returns {TaskDetails}
 */
deserializeDataviewTaskLine(line: string): TaskDetails;

This is all Kanban really needs, and would allow the implementation details to be under your control.

@claremacrae
Copy link
Collaborator

Totally fair. What if the serializer related API looked like this instead?

/**
 * Retrieves task details from a Tasks Emoji Formatted string
 *
 * @param line he markdown string of the task to be deserialized
 * @returns {TaskDetails}
 */
deserializeEmojiTaskLine(line: string): TaskDetails;

/**
 * Retrieves task details from a Dataview formatted string
 *
 * @param line he markdown string of the task to be deserialized
 * @returns {TaskDetails}
 */
deserializeDataviewTaskLine(line: string): TaskDetails;

This is all Kanban really needs, and would allow the implementation details to be under your control.

I appreciate the suggestion.... I still won't be able to try this out for a few days, but from reading the code, here are a few more thoughts...

By exposing TaskDetails, it still ties the hands of future maintenance.

That's because TaskDetails is an implementation detail of the current serialisation code - which was probably clear when you copied-and-pasted the docs in - every time a new field is added, somebody will have to come and update the API docs now, to add new fields.

Also, the toggleLine() function in the API presumably does honour any custom statuses in the user's Tasks settings - but the serialise/deseraliase functions do not.

So the interactions between other plugins and any user's custom statuses in Tasks may well be a source of hidden bugs.

So it all seems a bit confused to me.

What is published and documented for users is (a lot of) the Task class - via Task Properties - so an API based on strings and Task objects would feel more comfortable.

@claremacrae
Copy link
Collaborator

@mgmeyers, it would probably help to see how you are using this in the Kanban code... Is the code visible?

@mgmeyers
Copy link
Contributor Author

It is!

Here's where I extract task details from Kanban strings:

https://github.com/mgmeyers/obsidian-kanban/blob/c522b1a3134188412d9ca7d5d8cf6be4a49ce7e3/src/parsers/helpers/obsidian-tasks.ts#L135-L145

I remove any empty values from the object, and then this data eventually gets passed to this component that renders the task details:

https://github.com/mgmeyers/obsidian-kanban/blob/main/src/components/Item/DateAndTime.tsx#L175-L193

Here I first convert the key to an icon (e.g., dueDate -> 📅) then convert the value to a string if it isn't one already.

@mgmeyers
Copy link
Contributor Author

Just discovered this and wanted to add this to the discussion:

https://github.com/blacksmithgu/obsidian-dataview/blob/0c54ac0b3cc83e72ad40913b3229ff02923243d6/src/data-import/inline-field.ts#L182-L215

Looks like dataview is doing manual parsing of task emoji fields.

@mgmeyers mgmeyers changed the title feat: Expose api functionality needed for Kanban compatability feat: Expose api functionality needed for Kanban compatibility Apr 26, 2024
@mgmeyers
Copy link
Contributor Author

FYI, I've paired this down to just exposing the toggleLine function since the deserialization isn't as necessary for compatibility.

@claremacrae
Copy link
Collaborator

FYI, I've paired this down to just exposing the toggleLine function since the deserialization isn't as necessary for compatibility.

Thank you.

Copy link
Collaborator

@ilandikov ilandikov left a comment

Choose a reason for hiding this comment

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

Many thanks for your contribution. May we please ask for a couple of small changes to release this?

docs/Advanced/Tasks Api.md Outdated Show resolved Hide resolved
tests/Api/toggleLine.test.ts Outdated Show resolved Hide resolved
src/Commands/ToggleDone.ts Outdated Show resolved Hide resolved
docs/Advanced/Tasks Api.md Outdated Show resolved Hide resolved
src/Api/TasksApiV1.ts Outdated Show resolved Hide resolved
@mgmeyers
Copy link
Contributor Author

mgmeyers commented May 6, 2024

@ilandikov I've updated according to your suggestions. Let me know if you see any other issues.

@mgmeyers mgmeyers changed the title feat: Expose api functionality needed for Kanban compatibility feat: Add executeToggleTaskDoneCommand to Tasks Api May 6, 2024
Copy link
Collaborator

@ilandikov ilandikov left a comment

Choose a reason for hiding this comment

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

Looks great. Could you please take a look at one line of documentation and I think we are good to go =)

docs/Advanced/Tasks Api.md Outdated Show resolved Hide resolved
@mgmeyers mgmeyers requested a review from ilandikov May 7, 2024 23:46
@ilandikov ilandikov merged commit 63ad650 into obsidian-tasks-group:main May 8, 2024
1 check passed
@ilandikov
Copy link
Collaborator

Thanks @mgmeyers ! =)

@claremacrae
Copy link
Collaborator

Also just released in Tasks 7.2.0 - many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: for plugin developers Tools for use by plugin developers, including the Tasks API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants