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

Allow optional time in date metadata field #343

Merged
merged 22 commits into from
Aug 18, 2020
Merged

Conversation

clojj
Copy link
Contributor

@clojj clojj commented Aug 15, 2020

Fixes #257

TODOs:

  • use Alternative for that ugly code in parseZettelDate ?
  • unit tests

@clojj clojj changed the title Dev/tag datetime date-tag gets created as yyyy-mm-ddTHH:MM (old date format still valid) Aug 15, 2020
@srid srid changed the title date-tag gets created as yyyy-mm-ddTHH:MM (old date format still valid) Allow optional time in date metadata field Aug 15, 2020
@clojj
Copy link
Contributor Author

clojj commented Aug 16, 2020

 * [x?]  Drop `created` terminology and retain `date`. Rationale here: [#257 (comment)](https://github.com/srid/neuron/issues/257#issuecomment-672891322)

 * [x]  Change the Zettel field to `zettelDate :: Either Day LocalTime` (which incidentally is isomorphic to `(Day, Maybe TimeOfDay)`. We should not automatically add a time (midnight or midday) if the user has not specified one. Less magic, and more explicit types are a good thing.
   
   * Perhaps make a type alias first: `type DateMayTime = Either Day LocalTime`

 * [x]  Revert the visual regression to how `?timeline` gets rendered; i.e., it should display only day, not the time. However, make it display the full time of the day (if specified) on mouse hover (by setting the `title` HTML attribute).

 * [ ]  Add tests
   
   * Make sure that zettel sorting takes time into account, if specified.

 * [x]  Remove the "Created on:" at the bottom, because that can be confusing if date is not always creation date.

...posted here, just to pick up from the last PR

@clojj
Copy link
Contributor Author

clojj commented Aug 16, 2020

I guess you were referring to that sorting test in ZettelSpec ?

Copy link
Owner

@srid srid left a comment

Choose a reason for hiding this comment

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

Generally looks good, but will require some changes.

neuron/src/lib/Neuron/Zettelkasten/Zettel/Meta.hs Outdated Show resolved Hide resolved
neuron/src/lib/Neuron/Zettelkasten/Zettel/Parser.hs Outdated Show resolved Hide resolved
neuron/test/Neuron/Reader/OrgSpec.hs Outdated Show resolved Hide resolved
neuron/test/Neuron/Zettelkasten/ZettelSpec.hs Outdated Show resolved Hide resolved
neuron/test/Neuron/Zettelkasten/ZettelSpec.hs Outdated Show resolved Hide resolved
neuron/src/app/Neuron/CLI/Types.hs Outdated Show resolved Hide resolved
neuron/src/lib/Neuron/Zettelkasten/Zettel/Meta.hs Outdated Show resolved Hide resolved
neuron/src/lib/Neuron/Zettelkasten/Zettel/Meta.hs Outdated Show resolved Hide resolved
neuron/src/lib/Neuron/Zettelkasten/Zettel/Meta.hs Outdated Show resolved Hide resolved
neuron/src/app/Neuron/CLI/Types.hs Outdated Show resolved Hide resolved
@clojj
Copy link
Contributor Author

clojj commented Aug 17, 2020

@srid all comments resolved

@clojj
Copy link
Contributor Author

clojj commented Aug 17, 2020

The JSON API has to be changed or is that wip ?

@srid
Copy link
Owner

srid commented Aug 17, 2020

The JSON API has to be changed or is that wip ?

That is automatically taken care of by the Generic Aeson instance, when you modified the Zettel type by changing the field to zettelDate :: Maybe DateMayTime. I added the label only as a reminder for myself, in the context of Cerveau (which stores the JSON in postgres, and will require migration/reset when upgrading production)

EDIT: It is also to remind us that the associated change may impact editor support (emacs, vim) which uses neuron query's JSON output.

@srid srid merged commit a17e993 into srid:master Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend Metadata 'date' to include hours/minute/seconds ?
2 participants