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

Create insert journal headline at point #335

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dalanicolai
Copy link
Contributor

I have a project where I would like to use journal style headlines outside a journal file. This small adjustment enables the option to insert journal style headlines at point. I thought it might be useful sometimes for other user too. If you think the extra command is too "exclusive" to be added by default then it is no problem if you remove it. But then it would still be nice to keep the optional insert-at-point argument.

I have a project where I would like to use journal style headlines outside a journal file. This small adjustment enables the option to insert journal style headlines at point. I thought it might be useful sometimes for other user too. If you think the extra command is too "exclusive" to be added by default then it is no problem if you remove it. But then it would still be nice to keep the optional `insert-at-point` argument.
@bastibe
Copy link
Owner

bastibe commented Feb 22, 2021

I like it! Let's see what @cslux will say.

dalanicolai added a commit to dalanicolai/org-journal that referenced this pull request Feb 24, 2021
Currently the command inserts the time preceding the TODO. This PR implements
fixes (non-desctructively), but it assumes that bastibe#335 and bastibe#337 get merged
first.
dalanicolai added a commit to dalanicolai/org-journal that referenced this pull request Feb 24, 2021
Currently the command inserts the time preceding the TODO. This PR implements
fixes (non-desctructively), but it assumes that bastibe#335 and bastibe#337 get merged
first.
Currently the command inserts the time preceding the TODO. This PR implements
fixes (non-desctructively), but it assumes that bastibe#335 and bastibe#337 get merged
first.
@bastibe bastibe requested a review from casch-at March 6, 2021 13:19
Inserting journal style headlines easily can be useful in non-journal buffers
too. Especially when using any 'org-notifications' package (e.g.
[org-wild-notifier](https://github.com/akhramov/org-wild-notifier.el)).
@bastibe
Copy link
Owner

bastibe commented Apr 21, 2021

On a second thought, and a more thorough look at your code, I feel that this behavior does not fit well into org-journal. Reading through org-journal's code, it is not clear how this functionality is related to org-journal's behavior.

I think I would advise you to maintain a fork or a derivative project for this functionality.

That said, I would have no problem with adding an optional argument or configuration variable here and there for enabling your fork.

@dalanicolai
Copy link
Contributor Author

dalanicolai commented Apr 21, 2021

Okay thanks, that is fine also. On the other hand, all arguments I've added are optional, and should not break anything.

The only parts that could break something would be

  • moving the TODO in front of the time in the headline (in Improve new scheduled entry command #338), but as I wrote in that PR (with a link to org's documentation), that is required for org to handle the HEADLINE as a todo. For example, the current TODO headlines created with org-journal-new-scheduled-entry don't show up in org-todo-list.
  • adding the time within the time-stamp (but that was merged already by cslux, he (or me) just had to update a single test)
  • inserting the scheduled keyword when creating a scheduled entry

I would say these are nice little "improvements/corrections" (if indeed they do not break the functionality), but they are not essential at all of course. Anyway, no problem for me to maintain a fork.

@bastibe
Copy link
Owner

bastibe commented Apr 21, 2021

I saw that all the arguments are optional. My concern is more with the maintenance surface. Optional arguments only make sense if the code itself is actually using them--otherwise, how would you decide whether to delete some apparently unused piece of code?

Therefore, such affordances for third parties must be kept to a minimum, and rigorously documented (and tested, if possible). In case of the insert-at-point arguments, I felt that they needed to be introduced in a few too many places for comfort. Perhaps there is a way of refactoring org-journal's internals such that insertion and location are handled separately, and can be reused by your code more easily (just thinking out loud here). That would be a preferred solution to me.

As for the order of date and TODO, as well as the scheduled keyword, I am all in favor of your fixes in those instances.

All of that said, I am not really the maintainer of org-journal any longer. If @cslux later decides that this pull request is acceptable after all, I am entirely ok with that, too.

@casch-at casch-at added this to the 2.3.0 milestone Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants