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.

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

Added check to ignore --date-format option for hledger users #441

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

GLakovnik
Copy link

@GLakovnik GLakovnik commented Oct 31, 2024

This PR intends to improve ledger-mode compatibility for hledger users.
It requires no additional configuration and is completely invisible to ledger users while fixing the most common error hledger users see.

It's "the dumbest thing that works", a simple check whether ledger-binary-path points to hledger. If it does, ledger-exec-ledger does not include the --date-format option (which hledger doesn't have).

Solves the largest problem for hledger users and is a prerequisite for other improvements (WIP, i plan to make reconcilliation work as well and this is the first step.)
I've documented the incompatibilities in hledger's documentation.

With this change the following now works with hledger:
Quick balance display (<C-c> <C-p>)
Add transaction (<C-c> <C-a>)
Display ledger stats (<C-c> <C-l>)

@bcc32
Copy link
Collaborator

bcc32 commented Nov 15, 2024

Does hledger really not support setting a format for dates? I'd rather do something different than just "silently don't respect the customized value of --date-format" in that case---perhaps we could support setting ledger-default-date-format to nil, which hledger users would have to do?

@GLakovnik
Copy link
Author

GLakovnik commented Nov 16, 2024

EDIT: Edited for brevity

Hledger infers most formatting settings from input data -- transactions and rules, and allows for simple dates (%Y-%m-%d, %Y/%m/%d, %Y.%m.%d). While it does have a date-format option, it's a helper to define a format string for parsing dates from CSV files specifically and not used to specify the input/output format. Nothing is lost for hledger users by ignoring --date-format.

I've changed the PR, allowing setting ledger-default-date-format to nil. This caused a couple of additional minor changes through the codebase, namely:

  • ledger-default-date-format :type is now '(choice string (const nil))
  • so is ledger-reconcile-default-date-format
  • ledger-iso-date-format added to ledger-format-date as fallback

The following relates to other incompatibilities and possible future changes

The PR was mainly intended to get the ball rolling -- i wanted to propose something and see what the community/maintainer reaction would be. My goal here is to understand the code style expected so i can submit further PRs and reduce friction. Most incompatibility can be fixed by adjusting flags for ledger/hledger invocation. As an example, an incompatibility that got resolved in the past was the differences in color settings.

Ledger uses '--color --force-color', hledger uses '--color=always'.
This got resolved by creating the custom variable ledger-report-native-highlighting-arguments.
There are other similar differences which hledger users currently solve with a shell script that modifies command-line options before calling hledger, namely:

  • changing 'xact' to 'print --match'
  • changing 'cleared' to 'balance -N -C'

There are other equivalences related to reconciliation, but the usefulness of those is dependent on work i haven't done yet.
All of these along with a compatibility checklist are documented in hledgers documentation.

Can i use the 'introduce a custom variable' method for these differences as well?
Ideally i would like to submit 3 or 4 quick PRs to make these changes and then tackle reconcilliation -- i don't know what that entails yet.

#279 -- Relevant question by another hledger user about another one of these minor differences.

Thank you for your time reading all this, i hope to cause less distraction in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants