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

support custom date format #296

Merged
merged 17 commits into from Nov 2, 2021
Merged

support custom date format #296

merged 17 commits into from Nov 2, 2021

Conversation

shabegom
Copy link

@shabegom shabegom commented Aug 24, 2021

This PR adds support for:

  • custom date format for due and done dates
  • toggle to make due and done dates links for linking to DNP

There are two new settings added to Tasks settings modal. One allows a date format to be set which is then used during moment.format(). The other is a toggle to make dates links.

I've also updated the due and done date regex to be a bit more open. I went the easy route instead of trying to guess all the different date formats. I hope this is ok!

@TfTHacker
Copy link

@TfTHacker TfTHacker commented Aug 24, 2021

+1 up vote for this feature

@GitMurf
Copy link

@GitMurf GitMurf commented Aug 24, 2021

+99 up vote as well ;-) This is great! Thanks @shabegom for doing this and thanks @schemar for the amazing plugin!

@arminta7
Copy link

@arminta7 arminta7 commented Aug 24, 2021

+1

2 similar comments
@kmaustral
Copy link

@kmaustral kmaustral commented Aug 24, 2021

+1

@PJJPtx
Copy link

@PJJPtx PJJPtx commented Aug 25, 2021

+1

@schemar
Copy link
Owner

@schemar schemar commented Aug 25, 2021

Thank you @shabegom 🎉

I will do a proper review when I find the time. Until then, can you please:

  1. Make sure tests and linting pass
  2. Add documentation to the README
  3. Consider whether there is a better way to set the format on the Task from the settings (maybe only once when the plugin initializes? Maybe get the format always from the settings? Setting the static property doesn't look right in my opinion)

Thank you for the effort! ❤️

@shabegom
Copy link
Author

@shabegom shabegom commented Aug 25, 2021

@schemar:

  1. fixed linting errors and tests now pass. I added extra validation on due and done dates at the point they are removed from the description. This means the initial regex is a pretty broad match, but the result of the match must also match the proper date format in order to be considered a due or done date. I think this is a decent workaround vs trying to support all possible date formats via regex.
  2. Added a section on these new setting to the README. LMK if anything could be clearer there.
  3. I moved the format and linking setting to be something that is provided when constructing the class / accessed directly. I don't think this totally solves what you were getting at, but I'm not sure how I could read the format once on plugin initialization. Would love a point in the right direction if this change doesn't work.

LMK if you have any other feedback! That test allowing the emoji in the description got me good! If you want me to write any tests for this feature lmk.

@schemar schemar mentioned this pull request Aug 28, 2021
@schemar
Copy link
Owner

@schemar schemar commented Aug 28, 2021

I think you can remove the dateFormat property from the Task class as you will get the format from the settings every time.

However, I can't wrap my head around a fundamental problem when allowing users to change the format of dates: what happens to all the tasks already in the vault? Their due and done dates would simply become invalid and instead be part of the description now. In your current implementation this also means the parser stops parsing metadata from the end of the task line and thus the recurrence rule would be lost as well.

In short: just allowing users to change the format will lead to what they deem a "broken" Tasks plugin and get back to me via bug tickets.

We need to consider updating all existing tasks when changing the setting. 🤔 Or we only allow a certain set of formats that we all try to match (regardless of the setting) when reading the task. Only newly written tasks would then have the new format, e.g. when toggling, editing, or creating a task.

What do you think?

@shabegom
Copy link
Author

@shabegom shabegom commented Aug 30, 2021

Here's the simplest approach imo. I could add a button to the settings or a command that runs a one time conversion to any new date format specified. That way all previous tasks would get updated. it could also just auto update, but we'd need to validate the user has completed entering their new format into the field with onBlur or something...

I'm not too sure about accepting any date format, it's totally doable (just don't provide a second argument to moment() and any valid dateformat will be parsed). But I think it will confuse people if they see a different format between edit and preview of the tasks.

@kmaustral
Copy link

@kmaustral kmaustral commented Aug 30, 2021

@jan-willi
Copy link
Contributor

@jan-willi jan-willi commented Sep 3, 2021

While going down the rabbit hole to modify this plugin to be my perfect task manager I stumbled across the same problem of being able to read multiple formats.
I "solved" it by allowing to set multiple formats in the settings. All formats can be read, while the first supplied format will be used for new tasks. Is this something that could work?

The branch I'm working on (not at all pretty enough for a PR):
https://github.com/jan-willi/obsidian-tasks/tree/datetime
(Also includes custom signifiers and optional times in addition to dates. I might open separate PR's if this works out.)

@TfTHacker
Copy link

@TfTHacker TfTHacker commented Sep 19, 2021

Hoping to see this merge already for days :-)

I am also willing to migrate my tasks to the new format.

@schemar
Copy link
Owner

@schemar schemar commented Sep 22, 2021

I don't have time right now for private reasons. I plan to add some new things, including open PRs, to Tasks some time during October. Please be patient.

I think the best solution will be to not support any format, but a number of specific formats, including links ([[]]).

@schemar
Copy link
Owner

@schemar schemar commented Nov 1, 2021

Thank you for the update @shabegom. This is next on my list.

@shabegom
Copy link
Author

@shabegom shabegom commented Nov 1, 2021

The fork is still a broken given the massive update. I'm working on it though. Totally didn't realize I opened this PR off the main branch...

@schemar
Copy link
Owner

@schemar schemar commented Nov 2, 2021

The fork is still a broken given the massive update. I'm working on it though. Totally didn't realize I opened this PR off the main branch...

Thank you! If it is too much, I can also do it. I suspect I will do some changes after merging either way 👍

@schemar
Copy link
Owner

@schemar schemar commented Nov 2, 2021

It also looks like there are some unrelated changes @shabegom 🤔

@shabegom
Copy link
Author

@shabegom shabegom commented Nov 2, 2021

Do you have an example? I only changed things to get the custom date format and date links working with the new options. Anything else must have slipped in accidentally.

Oh maybe you mean the manifest? I can change that back. That was for my own testing. Sorry! 🙈

My tsserver was yelling at me about app not existing and Notice not being used...I can also revert that change if it was wrong. 🤦

@shabegom
Copy link
Author

@shabegom shabegom commented Nov 2, 2021

@schemar I'll revert those changes back today. Sorry about that. Just blindly following the linting errors.

@schemar
Copy link
Owner

@schemar schemar commented Nov 2, 2021

If the linter complains, it's fine to update 😄

@shabegom
Copy link
Author

@shabegom shabegom commented Nov 2, 2021

@schemar I've reverted some of the unrelated changes and left the ones that were throwing errors. Also updated the tests in Query.test.ts.

I'm getting a test failure that I don't think is related to the changes:

Jest: concurrent test "Query sorting instructions sorting as {"input":"sort by status\nsort by due","output":[{"property":"status","reverse":false
},{"property":"due","reverse":false}]}" must return a Promise.

Let me know if there is anything else I can do to get this merge ready. Thanks!

@schemar schemar changed the base branch from main to custom_date_format Nov 2, 2021
@schemar schemar self-assigned this Nov 2, 2021
@schemar schemar self-requested a review Nov 2, 2021
Copy link
Owner

@schemar schemar left a comment

Thanks a bunch @shabegom! I will use parts of this to get me a speedy start with the feature. It really helps a lot. I will do some things differently. For example, I am not sure I cannot maintain arbitrary date formats.

tasks.map(async (task) => {
if (task.description.startsWith('![[')) {
const link = parseLinktext(task.description);
const subpath = link.subpath
.replace('#^', '')
.replace(']]', '');
if (link) {
const file = this.app.metadataCache.getFirstLinkpathDest(
link.path.replace('![[', ''),
task.path,
);
const content =
file && (await this.app.vault.read(file)).split('\n');
const blocks =
file &&
this.app.metadataCache.getFileCache(file)?.blocks;
const line = blocks && blocks[subpath]?.position.start.line;
if (line) {
task.description = content[line]
.split(' ^')[0]
.replace('- ', '');
}
}
}
});
Copy link
Owner

@schemar schemar Nov 2, 2021

Choose a reason for hiding this comment

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

@shabegom can you please explain how this is related to the custom date format?

Copy link
Author

@shabegom shabegom Nov 2, 2021

Choose a reason for hiding this comment

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

This is so weird! This is from a totally unrelated plugin (block ref counter) and I have no idea how it made it in here. My best guess is Github Copilot suggested this code and I hit tab by accident...

I'm sorry I didn't catch this before. That's really embarrassing. can be fully deleted.

Copy link
Owner

@schemar schemar Nov 2, 2021

Choose a reason for hiding this comment

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

No worries

@@ -146,6 +172,7 @@ class QueryRenderChild extends MarkdownRenderChild {
}: {
tasks: Task[];
content: HTMLDivElement;
app: App;
Copy link
Owner

@schemar schemar Nov 2, 2021

Choose a reason for hiding this comment

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

is this actually used?

Copy link
Author

@shabegom shabegom Nov 2, 2021

Choose a reason for hiding this comment

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

I don't believe it is used. I missed this.

schemar
schemar approved these changes Nov 2, 2021
Copy link
Owner

@schemar schemar left a comment

Merging into a branch. It would still help me if you replied to my questions 😄 Thank you!

@schemar schemar merged commit 2a69927 into schemar:custom_date_format Nov 2, 2021
2 of 3 checks passed
@schemar
Copy link
Owner

@schemar schemar commented Nov 2, 2021

@shabegom, @TfTHacker, @GitMurf, @arminta7, @kmaustral, @PJJPtx, @jan-willi, @wenlzhang all of you upvoted or participated here or in the discussion #69.

If Tasks supported a fixed list of date formats (only dates, no times), which format(s) would you require? All would have the option to be links.

@TfTHacker
Copy link

@TfTHacker TfTHacker commented Nov 2, 2021

Thank you for your work on this project!!! Format for me:

YYYY-MM-DD

By the way, one of things I have done with my plugins is get the date format for daily notes page which tends to be the format used by the user in his vault. This can be easily retrieved with this library. https://github.com/liamcain/obsidian-daily-notes-interface

@schemar
Copy link
Owner

@schemar schemar commented Nov 2, 2021

By the way, one of things I have done with my plugins is get the date format for daily notes page which tends to be the format used by the user in his vault. This can be easily retrieved with this library. https://github.com/liamcain/obsidian-daily-notes-interface

This is great! Thank you!

schemar added a commit that referenced this issue Nov 2, 2021
Incorporating and cleaning up the changes proposed in #296.
@schemar schemar mentioned this pull request Nov 2, 2021
9 tasks
@kwjfef
Copy link

@kwjfef kwjfef commented Nov 10, 2021

Please could you add the option to change the date format to DD-MM-YYYY (reverse order) to follow EU/UK regional formatting?

@schemar
Copy link
Owner

@schemar schemar commented Nov 11, 2021

Please could you add the option to change the date format to DD-MM-YYYY (reverse order) to follow EU/UK regional formatting?

Will do 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants