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.

Sign up for GitHub

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

If startup option "nologrepeat" is set, don't log repeat information #119

Merged
merged 7 commits into from
Nov 25, 2019
Merged

If startup option "nologrepeat" is set, don't log repeat information #119

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b9f859
If startup option "nologrepeat" is set, don't log repeat information
jamesnvc Nov 25, 2019
3cf37ef
Add helper function to find inherited value of a property
jamesnvc Nov 25, 2019
dd6bf0f
Also honour :LOGGING: nologrepeat property
jamesnvc Nov 25, 2019
435591e
fix: Adhere to 'nologrepeat' if there's multiple STARTUP options set
munen Nov 25, 2019
a3b64a8
doc: nologrepeat startup option
munen Nov 25, 2019
7ef6c0c
Add some more tests for setting nologrepeat via property list
jamesnvc Nov 25, 2019
aa8cbfc
Invert return value of noLogRepeatEnabledP
jamesnvc Nov 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions src/components/OrgFile/OrgFile.unit.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { parseOrg, _parsePlanningItems } from '../../lib/parse_org';
import exportOrg from '../../lib/export_org';
import readFixture from '../../../test_helpers/index';
import { noLogRepeatEnabledP } from '../../reducers/org';

/**
* This is a convenience wrapper around paring an org file using
* This is a convenience wrapper around parsing an org file using
* `parseOrg` and then export it using `exportOrg`.
* @param {String} testOrgFile - contents of an org file
*/
Expand All @@ -17,7 +18,7 @@ function parseAndExportOrgFile(testOrgFile) {
return exportedFile;
}

describe('Unit Tests for org file', () => {
describe('Unit Tests for Org file', () => {
describe('Parsing', () => {
test("Parsing and exporting shouldn't alter the original file", () => {
const testOrgFile = readFixture('indented_list');
Expand Down Expand Up @@ -115,4 +116,23 @@ describe('Unit Tests for org file', () => {
});
});
});
describe('Reducers and helper functions', () => {
describe('"nologrepeat" configuration', () => {
test('Detects "nologrepeat" when set in #+STARTUP as only option', () => {
const testOrgFile = readFixture('schedule_with_repeater_and_nologrepeat');
const state = parseOrg(testOrgFile);
expect(noLogRepeatEnabledP({ state, headerIndex: 0 })).toBe(false);
});
test('Detects "nologrepeat" when set in #+STARTUP with other options', () => {
const testOrgFile = readFixture('schedule_with_repeater_and_nologrepeat_and_other_options');
const state = parseOrg(testOrgFile);
expect(noLogRepeatEnabledP({ state, headerIndex: 0 })).toBe(false);
});
test('Does not detect "nologrepeat" when not set', () => {
const testOrgFile = readFixture('schedule_with_repeater');
const state = parseOrg(testOrgFile);
expect(noLogRepeatEnabledP({ state, headerIndex: 0 })).toBe(true);
});
});
});
});
13 changes: 6 additions & 7 deletions src/lib/org_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,13 +212,12 @@ const tablePartContainsCellId = (tablePart, cellId) =>
const doesAttributedStringContainTableCellId = (parts, cellId) =>
parts
.filter(part => ['table', 'list'].includes(part.get('type')))
.some(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file contains only whitespace change from prettier and can be ignored during the review. Sorry for that.

part =>
part.get('type') === 'table'
? tablePartContainsCellId(part, cellId)
: part
.get('items')
.some(item => doesAttributedStringContainTableCellId(item.get('contents'), cellId))
.some(part =>
part.get('type') === 'table'
? tablePartContainsCellId(part, cellId)
: part
.get('items')
.some(item => doesAttributedStringContainTableCellId(item.get('contents'), cellId))
);

export const headerThatContainsTableCellId = (headers, cellId) =>
Expand Down
129 changes: 68 additions & 61 deletions src/reducers/org.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,73 +1033,80 @@ function updatePlanningItemsWithRepeaters(
newTodoState,
currentTodoState
) {
{
indexedPlanningItemsWithRepeaters.forEach(([planningItem, planningItemIndex]) => {
state = state.setIn(
['headers', headerIndex, 'planningItems', planningItemIndex, 'timestamp'],
applyRepeater(planningItem.get('timestamp'), new Date())
);
});
indexedPlanningItemsWithRepeaters.forEach(([planningItem, planningItemIndex]) => {
state = state.setIn(
['headers', headerIndex, 'titleLine', 'todoKeyword'],
currentTodoSet.get('keywords').first()
['headers', headerIndex, 'planningItems', planningItemIndex, 'timestamp'],
applyRepeater(planningItem.get('timestamp'), new Date())
);
const startupOptNoLogRepeat = state
.get('fileConfigLines')
.some(elt => elt.match(/^#\+STARTUP:\s+nologrepeat/));
const loggingProp = inheritedValueOfProperty(state.get('headers'), headerIndex, 'LOGGING');
if (
!startupOptNoLogRepeat &&
!(
loggingProp &&
loggingProp.some(
v => v.get('type') === 'text' && v.get('contents').match(/\s*nologrepeat\s*/)
)
)
) {
const lastRepeatTimestamp = getCurrentTimestamp({ isActive: false, withStartTime: true });
const newLastRepeatValue = [
{
type: 'timestamp',
id: generateId(),
firstTimestamp: lastRepeatTimestamp,
secondTimestamp: null,
},
];

state = state.updateIn(['headers', headerIndex, 'propertyListItems'], propertyListItems =>
propertyListItems.some(item => item.get('property') === 'LAST_REPEAT')
? propertyListItems.map(item =>
item.get('property') === 'LAST_REPEAT'
? item.set('value', fromJS(newLastRepeatValue))
: item
)
: propertyListItems.push(
fromJS({
property: 'LAST_REPEAT',
value: newLastRepeatValue,
id: generateId(),
})
)
);
state = state.updateIn(['headers', headerIndex], header => {
let rawDescription = header.get('rawDescription');
if (rawDescription.startsWith('\n')) {
rawDescription = rawDescription.slice(1);
}
rawDescription =
`\n- State "${newTodoState}" from "${currentTodoState}" ${renderAsText(
fromJS(lastRepeatTimestamp)
)}\n` + rawDescription;
return header
.set('rawDescription', rawDescription)
.set('description', parseRawText(rawDescription));
});
}
});
state = state.setIn(
['headers', headerIndex, 'titleLine', 'todoKeyword'],
currentTodoSet.get('keywords').first()
);
if (noLogRepeatEnabledP({ state, headerIndex })) {
const lastRepeatTimestamp = getCurrentTimestamp({ isActive: false, withStartTime: true });
const newLastRepeatValue = [
{
type: 'timestamp',
id: generateId(),
firstTimestamp: lastRepeatTimestamp,
secondTimestamp: null,
},
];

state = state.updateIn(['headers', headerIndex, 'propertyListItems'], propertyListItems =>
propertyListItems.some(item => item.get('property') === 'LAST_REPEAT')
? propertyListItems.map(item =>
item.get('property') === 'LAST_REPEAT'
? item.set('value', fromJS(newLastRepeatValue))
: item
)
: propertyListItems.push(
fromJS({
property: 'LAST_REPEAT',
value: newLastRepeatValue,
id: generateId(),
})
)
);
state = state.updateIn(['headers', headerIndex], header => {
let rawDescription = header.get('rawDescription');
if (rawDescription.startsWith('\n')) {
rawDescription = rawDescription.slice(1);
}
rawDescription =
`\n- State "${newTodoState}" from "${currentTodoState}" ${renderAsText(
fromJS(lastRepeatTimestamp)
)}\n` + rawDescription;
return header
.set('rawDescription', rawDescription)
.set('description', parseRawText(rawDescription));
});
}
return state;
}

/**
* Is the `nologrepeat` feature enabled for this buffer?
* More info:
* https://www.gnu.org/software/emacs/manual/html_node/org/Repeated-tasks.html
*/
export function noLogRepeatEnabledP({ state, headerIndex }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be called logRepeatEnabledP instead, since it returns true if it should repeat and false if nologrepeat is set?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 /* }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, my, of course! It's a little late for me here, I apologize.

I should take a break, maybe(;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😄 Can't hurt! I can make that change & push it up

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

const startupOptNoLogRepeat = state
.get('fileConfigLines')
.some(elt => elt.match(/^#\+STARTUP:.*nologrepeat.*/));
const loggingProp = inheritedValueOfProperty(state.get('headers'), headerIndex, 'LOGGING');
return (
!startupOptNoLogRepeat &&
!(
loggingProp &&
loggingProp.some(
v => v.get('type') === 'text' && v.get('contents').match(/\s*nologrepeat\s*/)
)
)
);
}

/**
* Function wrapper around `updateCookiesOfHeaderWithId` and
* `updateCookiesOfParentOfHeaderWithId`.
Expand Down
2 changes: 2 additions & 0 deletions test_helpers/fixtures/schedule_with_repeater.org
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
* TODO Header with repeater
SCHEDULED: <2019-11-27 Wed +1d>
4 changes: 4 additions & 0 deletions test_helpers/fixtures/schedule_with_repeater_and_nologrepeat.org
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#+STARTUP: nologrepeat

* TODO Header with repeater
SCHEDULED: <2019-11-27 Wed +1d>
4 changes: 4 additions & 0 deletions test_helpers/fixtures/schedule_with_repeater_and_nologrepeat_and_other_options.org
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#+STARTUP: showeverything nologrepeat

* TODO Header with repeater
SCHEDULED: <2019-11-27 Wed +1d>