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

Jump to bottom

Support stdin journal (web), create journal file if not exists (ui, web) #978

Closed
wants to merge 2 commits into from
Closed

Support stdin journal (web), create journal file if not exists (ui, web) #978

wants to merge 2 commits into from

Conversation

zarybnicky
Copy link
Contributor

Closes #855. The hledger-web half of #950.

This also fixes some oversights regarding "-" journal files in hledger-lib and
hledger (some, not all).

The behavior with -f - is currently the following: the journal is loaded into
memory, and any /add commands append to the in-memory journal. /edit doesn't do
anything yet as readJournalFiles uses readFileOrStdinPortably and assumes
the file is either - or a file, so there's some postprocessing (filters, anon,
...) that would need to be reimplemented.

The #950 part is just using ensureJournalFileExists in place of
requireJournalFileExists.

@simonmichael simonmichael added the web The hledger-web tool. label Feb 21, 2019
@zarybnicky
Copy link
Contributor Author

I forgot to mention - it it possible to implement this without touching hledger-lib or hledger, but there would be a bit more special-casing required.

Looking back, I'm not sure about the writeValidJournal change, esp. if it shouldn't be a pure () instead, but I was looking at appendToJournalFileOrStdout at that time...

@zarybnicky
Copy link
Contributor Author

Completing /edit and /upload would also be a good step that I'd wanted to implement but ran out of time yesterday. I'm not sure if I should continue in this PR (and mark this one as WIP) or open another one later.

@zarybnicky
Copy link
Contributor Author

Oh, and also - the exceptions ("handle is already closed") were caused by the check in journalSpecifiedFileIsNewer, not the initial loading of the file.

@simonmichael
Copy link
Owner

simonmichael commented Feb 22, 2019 via email

@zarybnicky zarybnicky mentioned this pull request Feb 28, 2019
Merged
simonmichael
simonmichael requested changes Mar 1, 2019
View reviewed changes
Copy link
Owner

@simonmichael simonmichael left a comment

Choose a reason for hiding this comment

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

Oh.. maybe I did't submit this review...

hledger-web/Hledger/Web/Main.hs Outdated Show resolved Hide resolved
hledger-web/Hledger/Web/Widget/Common.hs Outdated Show resolved Hide resolved
@zarybnicky zarybnicky force-pushed the web-stdin-journal branch 3 times, most recently from 817b2c6 to 5cebaa4 Compare March 2, 2019 21:20
@zarybnicky
Copy link
Contributor Author

Sorry for the many force-pushes, I've been doing the changes one line at a time. I've rebased the commit and addressed the change requests, I think.

I'd like to implement in-memory parts of /edit and /upload as well, please don't merge this until then.

@zarybnicky zarybnicky changed the title web: Allow stdin journal (/add appends in-memory), create journal file if not exists WIP: web: Allow stdin journal (/add appends in-memory), create journal file if not exists Mar 2, 2019
@zarybnicky zarybnicky changed the title WIP: web: Allow stdin journal (/add appends in-memory), create journal file if not exists Support stdin journal (web), create journal file if not exists (ui, web) Mar 3, 2019
@zarybnicky
Copy link
Contributor Author

Now also closes #950.

I've split the PR into two commits, one adding support for stdin to hledger-web, the other creating non-existent journals on startup. I've also added the missing /edit and /upload parts - now all of hledger-web supports stdin journals, and also filled in the missing ensureJournalFileExists line to hledger-ui's main.

@simonmichael
Copy link
Owner

Sorry for the delay on this.

  • Have you used the hledger-web stdin support a bit ? Working fine ?
  • Why is the "create journal file if not exists" in this PR ? Are you sure that functionality is a good idea ?

@simonmichael
Copy link
Owner

Why is the "create journal file if not exists" in this PR ? Are you sure that functionality is a good idea ?

Related: in what situations does hledger do that already ? I think only hledger add, right ?

@simonmichael simonmichael mentioned this pull request May 21, 2019
Open
@simonmichael
Copy link
Owner

Hi @zarybnicky, any thoughts on this ? It says I requested a change, I guess that was to remove/postpone the auto-create-missing-journal-file functionality.

@zarybnicky
Copy link
Contributor Author

Oops, it seems I'd managed to archive the GitHub mail that has served as my reminder for this issue... I'll try to look it over tomorrow and see where it's at.

@simonmichael simonmichael added the needs:discussion To unblock: needs more discussion/review/exploration label Jul 16, 2019
@simonmichael
Copy link
Owner

Ping

@zarybnicky
Copy link
Contributor Author

Wow, I've been terrible regarding this PR. Real life stuff does that, sorry... I'll close this PR and open two separate ones instead

@zarybnicky zarybnicky closed this Aug 9, 2019
@simonmichael simonmichael mentioned this pull request Feb 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonmichael simonmichael simonmichael requested changes

Assignees
No one assigned
Labels
needs:discussion To unblock: needs more discussion/review/exploration web The hledger-web tool.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

better hledger-web first startup experience
2 participants
@zarybnicky @simonmichael