feat: Implementation of Task-Duration (#1649) #3464
Conversation
|
Thank you for this! I see you've force-pushed already, so I won't start reviewing yet, as any draft comments are invalidated (disconnected from the code) by force pushes... So for now I'll just have a quick experiment on my Mac to see what the chosen Emoji looks like. Please let me know when it's stable for a day or so, and I'll start reviewing. |
|
PS For future reference - I would definitely recommend that you create a branch for PRs in future, rather than pushing from the default branch. It will be easier for you and for maintainers. |
|
|
@Drezil I remain grateful for this contribution, but I am getting a little concerned about the size of this PR, before I have had a chance to review and give feedback on the initial code. There are things needing to be improved about the design of the first layer - the storage, reading and writing of durations - and the behaviour of the Duration class. The more code you add above the initial implementation of that layer:
Please do not add any more layers until you have let me know that the functionality you have now pushed is ready for review, and we have finalised what it looks like. Thank you again for all your work. |
|
Yes, reflecting on this, the design decisions about the lower layer will affect some aspects of the desired behaviour of the higher layers. So, when you say the code added in the initial commit is ready review, i will first do an initial review of that code. Once that has settled down, I'll then review searching/sorting/grouping. |
|
Whilst I'm thinking about reviews, a general comment about how review Conversations are used in the project: FYI I use the "conversation resolved" facility to mean that any discussion is completed and/or requested changes have been made and reviewed. This enables me to keep track of what remains, whereas if the contributor marks conversations as being resolved, all discussion is hidden by github and it is easy for me to miss comments I needed to read, or changes I needed to review.... So in summary, as you respond to any review comments, please do leave a note to say when things have been done - but just please don't mark conversations as resolved - thanks in advance. |
claremacrae
added
type: enhancement
|
Hi @Drezil, how are you getting on - what's the status of this? Cheers. |
|
I am testing this extension by dogfooding it in my daily routine. But i got not much time at the moment (currently moving with family to another city). I still have 1-2 issues that i want to correct (especially with completion while creating tasks), do a proper rebase onto current master and then tackle everything related to document things properly (and link to those remarks in code-comments). |
|
Great. Thank you. Good luck with the move. Gentle reminder to create a branch from the latest main/master and put your changes on that branch, instead of your default branch. |
|
It would be really helpful for the review and merge process if the PR could be created from a branch instead of your |
|
Hi @Drezil, Great to see that you've caught this up with the latest code - many thanks for that. I know that it's still a draft - but I've started looking through the code, and naturally, for a PR of this size, have some feedback. But I'm not sure when to actually start reviewing. Certainly, once I start reviewing it will be important that there are no more force-pushes, as all the review comments would effectively be lost, as they would refer to commits that no longer existed on your branch. So, from my perspective, ideally:
Later on, once the behaviour is relatively finalised, we can start looking at getting documentation written. |
|
Would really love to see this one in the plugin! - great effort, maybe worth releasing as a beta? |
Hi @Drezil, I hope you are OK. Whilst the suggested beta wouldn't be practical or maintainable, I very aware of the value of this and very much willing to do work on getting this to a releasable state. What's your current plan please? if you aren't going to do any more work on it, I plan at some point to mark the PR as ready for review, and then:
|
|
|
rebased it. So far stable. if someone else want to do that it is fine, too. |
|
Super - thanks very much. So from this point on, please now don't force-push - so that I can start reviewing the changes as and when I have free time... (I'll initially make draft review comments - and force-pushing would lose any draft comments I had made, meaning I would need to start the review again...) |
|
What's the best way to install/test this? - I'm new to Obsidian so would appreciate a nudge in the right direction :) |
Thank you. I would welcome any exploratory testing that you could do - are you able add, edit and search for durations? You can install a build of this Pull Request by:
|
3 participants
Types of changes
Changes visible to users:
feat- non-breaking change which adds functionality)feat!!orfix!!- fix or feature that would cause existing functionality to not work as expected)fix- non-breaking change which fixes an issue)i18n- additions or improvements to the translations - see Support a new language)docs- improvements to any documentation content for users)vault- improvements to the Tasks-Demo sample vault)contrib- any improvements to documentation content for contributors - see Contributing to Tasks)Internal changes:
refactor- non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)test- additions and improvements to unit tests and the smoke tests)chore- examples include GitHub Actions, issue templates)Description
Step 1 of #1649 (comment)
Motivation and Context
#1649
How has this been tested?
Currently dogfeeding this PR in my daily obsidian. Will test it extensively - but added it to all tests-suites and i am pretty confident that no issues should arise.
Screenshots (if appropriate)
Checklist
yarn run lint.Remaining Issues
this draft-PR gets updated if things tag along. See it as a first preview.
Terms