-
Notifications
You must be signed in to change notification settings - Fork 116
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
add ability to list note timestamps #626
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nicknickel, thanks for the effort for trying to close the long living feature request #90. The code is pretty reasonable.
Before talking about code, I would like to discuss how this project should decide whether to add a feature. I should have clarified it way before, so it's my bad. I didn't do it because this project didn't receive feature contributions very frequently.
When adding new code, I think we must ensure that it is long-term, simple, and open. I added the following tenets in the project wiki to provide more detail: https://github.com/dnote/dnote/wiki#tenets. Let me know what you think.
The timestamp could be useful to some users. Since I don't have a solid data to back that up, I would like to go back to the long-term and simple tenet. As a start, could you describe how useful you would find this feature in your experience?
Tagging @robertmartin8 and @colearendt who originally interacted with #90. Please let me know how you feel about this change, if it is still relevant. Thanks.
pkg/cli/cmd/ls/ls.go
Outdated
preface := log.ColorYellow.Sprintf("(%d)", info.RowID) | ||
if timestamps { | ||
local_time := time.Unix(0, info.AddedOn).Format(time.DateTime) | ||
preface = log.ColorYellow.Sprintf("(%s)", local_time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about showing the ID as well as the timestamp?
dnote v --timestamps mybook
(1) [2023-02-22 00:00:01] note 1 with a multiple lines of content [---More--]
(2) [2023-02-22 00:00:02] note 2
Might be helpful in case we want to expand the contents of a note
dnote v 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it. I went ahead and updated the output to look like you indicated above.
pkg/cli/cmd/ls/ls.go
Outdated
if timestamps { | ||
local_time := time.Unix(0, info.AddedOn).Format(time.DateTime) | ||
preface = log.ColorYellow.Sprintf("(%s)", local_time) | ||
} | ||
if isExcerpt { | ||
body = fmt.Sprintf("%s %s", body, log.ColorYellow.Sprintf("[---More---]")) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for formatting the note content seems to be getting more complicated. What do you think about creating a separate function, and unit testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on both fronts. I went ahead and created a separate print note line function like what's there for printing book lines. I also added some tests that execute a view on notes. I added 3 tests which may be overkill. Let me know if that's too much.
@sungwoncho - This feature isn't critical to my workflow and I would consider it a nice-to-have; information I would find neat but not ultimately obstructing my usage. But, for what it's worth, I don't follow the same workflow mentioned in the issue this addresses so getting more feedback from other users would be good. I think those are solid tenets for this project. I would imagine that not implementing certain features that add complexity might detract some users from adopting dnote but other users would be attracted because of the tenets. Ultimately you can't please everyone so having tenets will help make those decisions. |
Thanks for the feedback. Let us wait for more evidence that users need this feature, so that we can make reasonable trade-off based on the tenets. |
Overview
This PR addresses #90 by adding a flag to the
view
subcommand that lists the note's creation timestamps rather than the note IDs. I know that issue is pretty old at this point but it seemed like a neat feature and simple enough to implement. Even so, I don't mind if you don't want to merge this.Show and tell
Output of
dnote view --help
for referenceThe resulting output would look like the below.