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

Finish removing journalFinalise #903

Merged
merged 3 commits into from Oct 16, 2018

Conversation

Projects
None yet
2 participants
@jkr
Contributor

jkr commented Oct 12, 2018

This finishes removing journalFinalise, and adds a fix in the order of operations.

[Followon from #893, #900]

jkr added some commits Oct 12, 2018

journal: Get rid of `journalFinalise` and use granular functions
Complete the process started in 53b3e2b. This gets rid of the
`journalFinalise` function and uses the smaller steps, in order to
have more granular control.
journal: Change order of operations in finalization
We want to make sure that we add the filepath after the order is
reversed, so the added filepath is on the head and not the tail (as it
would be if it were reversed after it was added).
@simonmichael

Sorry if I jumped the gun on the previous PR.

Do you want to also remove parseAndFinaliseJournal' ?

@@ -532,33 +534,14 @@ journalReverse j =
,jmarketprices = reverse $ jmarketprices j
}
-- | Set clock time in journal

This comment has been minimized.

@simonmichael

simonmichael Oct 12, 2018

Owner

"Set this journal's last read time, ie when its files were last read." ? Rename to journalSetLastReadTIme ?

This comment has been minimized.

@jkr

jkr Oct 12, 2018

Contributor

Sure -- I'll do another renaming/redoc-ing commit for this and the below, and add it to this PR.

journalSetTime :: ClockTime -> Journal -> Journal
journalSetTime t j = j{ jlastreadtime = t }
-- | See filepath in journal

This comment has been minimized.

@simonmichael

simonmichael Oct 12, 2018

Owner

"Add a file (and its content) to the journal's list of parsed files. (Does not parse the file or change the journal otherwise.)" ? Rename to journalAddFile or similar ? Move next to journalFilePaths ?

This comment has been minimized.

@jkr

jkr Oct 12, 2018

Contributor

Actually, there's already a redundant journalAddFile in Read.Common. The only difference is that it adds to the end. We don't need both, since we can just use the one on Read.Common and put it prior to the reversal.

This comment has been minimized.

@jkr

jkr Oct 12, 2018

Contributor

Addressed in 0d2f8c4 below.

@jkr

This comment has been minimized.

Contributor

jkr commented Oct 12, 2018

Do you want to also remove parseAndFinaliseJournal' ?

Not sure if I understand the full implications of that. I've been keeping it parallel so it would be easy to remove, but I haven't looked at the time-tracking code enough to know why it needed the different behavior.

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Oct 12, 2018

Ok no problem. I/we can look at that another time.

journal: Refine granular finalization functions
This commit fixes two of the granular finalization functions:

1. Rename `journalSetTime` to `journalSetLastReadTime` and improve
   documentation.

2. Remove `journalSetFilePath`. It's redundant with `journalAddFile`
   currently in `Hledger.Read.Common`. The only difference between the
   functions is where the file is added (we keep the one in which it
   is added to the tail), so we change the position vis-a-vis
   reversal.
@jkr

This comment has been minimized.

Contributor

jkr commented Oct 12, 2018

I think this is ready to go, from my end.

@simonmichael simonmichael merged commit a6a73e3 into simonmichael:master Oct 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@simonmichael

This comment has been minimized.

Owner

simonmichael commented Oct 16, 2018

Can't remember why I was waiting on this. I took a chance and squashed it into one commit, which I think worked out ok. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment