If startup option "nologrepeat" is set, don't log repeat information #119
Conversation
| @@ -212,13 +212,12 @@ const tablePartContainsCellId = (tablePart, cellId) => | |||
| const doesAttributedStringContainTableCellId = (parts, cellId) => | |||
| parts | |||
| .filter(part => ['table', 'list'].includes(part.get('type'))) | |||
| .some( | |||
There was a problem hiding this comment.
This file contains only whitespace change from prettier and can be ignored during the review. Sorry for that.
|
Hi @jamesnvc As always, great work! I did find a small issue: When there's another startup option set (for example Here's my proposal to fix it: 435591e However, it's your PR, so I'm not merging before you are OK with my proposal. Feel free to change anything, of course^^ In any case, thank you for proposing and implementing yet another great feature! 🙏 🙇♂️ |
| * More info: | ||
| * https://www.gnu.org/software/emacs/manual/html_node/org/Repeated-tasks.html | ||
| */ | ||
| export function noLogRepeatEnabledP({ state, headerIndex }) { |
There was a problem hiding this comment.
Should this be called logRepeatEnabledP instead, since it returns true if it should repeat and false if nologrepeat is set?
There was a problem hiding this comment.
Good question! I was actually thinking about this name for a moment when I had the same idea as you^^
I'm not super happy about the name, because it's quite long, but here's my thought process: The native Org feature is called nologrepeat. Hence it makes sense to call the feature flag in organice by the same name and not invert the native feature flags name. The feature flag is enabled when "nologrepeat" is 'enabled' or 'set'.
Atm it's kinda hard to read and if I would do it from scratch, I'd call the feature "logrepeatenabled", but Org mode has prior art here(;
On the other hand, having you trip up over the same naming issue that I was already thinking about is not a great argument for the name. So I'm open for a change if after reading this you have a strong fealing it woud be better the other way around!
There was a problem hiding this comment.
What if we keep the name, but invert what it returns, so when nologrepeat is set it returns true? That way we keep the name, but it reads a bit more naturally; e.g. the check for logging the repeat information would be if (!noLogRepeatEnabledP(...)) { /* log repeat /* }
There was a problem hiding this comment.
Oh, my, of course! It's a little late for me here, I apologize.
I should take a break, maybe(;
There was a problem hiding this comment.
😄 Can't hurt! I can make that change & push it up
There was a problem hiding this comment.
Sweet!
Also thanks for adding the test for detecting via properties. I was just starting to type that one in when I got the notification about your commit(;
I'll happily pull the changes in when you ping me.
|
Thanks for the feedback & tests @munen! I think things are looking good now. |
Address #118