Allow editing of DayOne entries #1001

wren · 2020-07-11T22:52:47Z

Fixes #955

DayOneJournal previously reimplemented Journal._parse inside of DayOneJournal.parse_editable_string, and in doing so caused issues between the two classes (the body was being reparsed for a title, overwriting the title, and essentially dropping the first line of each edited entry). This commit fixes the issue by moving the UUID to be in the body of the entry, rather than above it. Doing this allows us to use Journal._parse like normal (it still finds the correct boundaries between entries), and then DayOneJournal parses the UUID afterward.

Co-authored-by: MinchinWeb
Co-authored-by: Micah Jerome Ellison

Checklist

I have read the contributing doc.
I have included a link to the relevant issue number.
I have tested this code locally.
I have checked to ensure there aren't other open pull requests
for the same issue.
I have written new tests for these changes, as needed.
All tests pass.

Add test for editing Dayone entries (this test currently fails) Co-authored-by: Jonathan Wren <jonathan@nowandwren.com>

DayOneJournal previously reimplemented Journal._parse inside of DayOneJournal.parse_editable_string, and in doing so caused issues between itself and the class it was inheriting from. This commit fixes the issue by moving the UUID to be in the body of the entry, rather than above it. So, then Journal._parse still finds the correct boundaries between entries, and DayOneJournal then parses the UUID afterward. Co-authored-by: Micah Jerome Ellison <micah.jerome.ellison@gmail.com>

wren · 2020-07-11T23:02:59Z

Well, that took longer than expected, but I think this is all working now.

@MinchinWeb @micahellison Would y'all mind taking a look over this?

MinchinWeb · 2020-07-12T04:30:56Z

I haven't had the chance to test drive this yet, but it we're using a regex match:

UUID's should all be the same length. Should the regex enforce that?
should we be more explicit with our inserted text, perhaps by prefacing it with "UUID:"?
"#" is used for Level 1 headings in Markdown, but this is now at the bottom of the entry. Would it make sense to to move to an HTML comment style? Like 
- I realize not everyone writes their entries in Markdown, but I do. If for some reason the comment got included in the output, it would automatically just not be rendered in HTML, which I think it a good default.
- if you combine these two, you'd get 
should we think about extend-ability of this for other DayOne metadata? That doesn't have to be dealt with now, but (for example) currently you can't edit the tags on an entry in a straightforward, deterministic manner. If we went with the HTML comment style, it could be extended for tags, and potentially other metadata. So add another line like:

wren · 2020-07-12T22:20:15Z

@MinchinWeb I think those are all very good questions, and we should definitely discuss if and how to handle extended metadata on entries for certain journal types, but I would consider that out of scope for issue #955.

I was also very tempted to change the # to something more friendly, but I decided against it in order to keep the format as close to its current look. That way, we can take some time to plan any upcoming changes in a different issue.

MinchinWeb · 2020-07-14T20:54:13Z

jrnl/DayOneJournal.py

-                # merge existing tags with tags pulled from the entry body
-                entry.tags = list(set(entry.tags + match.tags))
-
-                # extended Dayone metadata


How is this extended metadata being preserved? or is it being dropped when the entry is edited?

It is being preserved: it is read when the journal is opened, it remains in memory as the entry is being edited, then it's saved back. And it still creates the metadata when writing a new entry.

MinchinWeb · 2020-07-14T20:55:21Z

jrnl/Entry.py

@@ -37,18 +39,30 @@ def title(self):
            self._parse_text()
        return self._title

+    @title.setter


These setters I don't think are being used by your solution. Would it make sense to drop them?

I think it's good to leave them in. It sends a nudge to future contributors that they should use entry's accessors rather than underscore-prefixed internals.

MinchinWeb · 2020-07-15T03:35:21Z

Simple local testing seems to show that it works! Extended metadata stays in place. The behave test suite passes locally.

In regards to this PR, should we remove the setter code I added, as it's not being used?

In regards to future work:

we should look at expanding this to support editing entry tags
we should look at the format of the UUID within the editing output
the "Device Agent" is currently being set to a empty string, which is less than ideal (this is not new)

I'm excited to see this merged. Thank for everyone's help!

LGTM!

micahellison

Looks good! Nice collaboration!

* add test to repro issue #955 * Allow editing of DayOne entries * Add broken test for Dayone Add test for editing Dayone entries (this test currently fails) Co-authored-by: Jonathan Wren <jonathan@nowandwren.com> * Fix editing logic for DayOneJournal DayOneJournal previously reimplemented Journal._parse inside of DayOneJournal.parse_editable_string, and in doing so caused issues between itself and the class it was inheriting from. This commit fixes the issue by moving the UUID to be in the body of the entry, rather than above it. So, then Journal._parse still finds the correct boundaries between entries, and DayOneJournal then parses the UUID afterward. Co-authored-by: MinchinWeb <w_minchin@hotmail.com> Co-authored-by: Micah Jerome Ellison <micah.jerome.ellison@gmail.com>

wren and others added 4 commits July 2, 2020 00:14

add test to repro issue jrnl-org#955

6f7c538

Allow editing of DayOne entries

07bacab

Add broken test for Dayone

efd5152

Add test for editing Dayone entries (this test currently fails) Co-authored-by: Jonathan Wren <jonathan@nowandwren.com>

wren changed the title ~~Fix Dayone editing~~ Allow editing of DayOne entries Jul 12, 2020

This was referenced Jul 12, 2020

Add test to repro issue #955 #989

Closed

Add DayOne edit tests #993

Closed

Allow editing of DayOne entries (#955) #990

Closed

MinchinWeb reviewed Jul 14, 2020

View reviewed changes

micahellison approved these changes Jul 18, 2020

View reviewed changes

micahellison added bug Something isn't working day one Issues related to Day One (dayoneapp.com) labels Jul 18, 2020

micahellison merged commit 9ccb0b9 into jrnl-org:develop Jul 18, 2020

wren deleted the dayone-edit-fix-955 branch October 18, 2020 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow editing of DayOne entries #1001

Allow editing of DayOne entries #1001

wren commented Jul 11, 2020 •

edited

Loading

wren commented Jul 11, 2020

MinchinWeb commented Jul 12, 2020 •

edited

Loading

wren commented Jul 12, 2020

MinchinWeb Jul 14, 2020

micahellison Jul 18, 2020

MinchinWeb Jul 14, 2020

micahellison Jul 18, 2020

MinchinWeb commented Jul 15, 2020

micahellison left a comment

Allow editing of DayOne entries #1001

Allow editing of DayOne entries #1001

Conversation

wren commented Jul 11, 2020 • edited Loading

Checklist

wren commented Jul 11, 2020

MinchinWeb commented Jul 12, 2020 • edited Loading

wren commented Jul 12, 2020

MinchinWeb Jul 14, 2020

Choose a reason for hiding this comment

micahellison Jul 18, 2020

Choose a reason for hiding this comment

MinchinWeb Jul 14, 2020

Choose a reason for hiding this comment

micahellison Jul 18, 2020

Choose a reason for hiding this comment

MinchinWeb commented Jul 15, 2020

micahellison left a comment

Choose a reason for hiding this comment

wren commented Jul 11, 2020 •

edited

Loading

MinchinWeb commented Jul 12, 2020 •

edited

Loading