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

New 'ledger-indent-region' function #399

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jdek
Copy link

@jdek jdek commented Feb 24, 2024

  • ledger-mode.el (ledger-mode): Set indent-region-function.
  • ledger-post.el (ledger-indent-region): New function that indents the region by calling ledger-indent-line for each line.

Closes #197

* ledger-mode.el (ledger-mode): Set indent-region-function.
* ledger-post.el (ledger-indent-region): New function that indents the
region by calling ledger-indent-line for each line.

Closes bug ledger#197

Signed-off-by: J. Dekker <[email protected]>
@jdek
Copy link
Author

jdek commented Mar 8, 2024

@purcell hi, possible to get a quick review on this? Would be quite nice to have this fixed in master.

@purcell
Copy link
Member

purcell commented Mar 11, 2024

Probably @enderw88 would be better placed to comment on whether this is a desirable fix — I'm not sure about the history of indentation in ledger-mode

@jdek
Copy link
Author

jdek commented Mar 31, 2024

Maybe @bcc32 could take a look?

@purcell
Copy link
Member

purcell commented Apr 1, 2024

As far as I can tell, the default indent-region-functionindent-region-line-by-line – calls indent-line-function on each line. So what's the difference here, assuming that indent-line-function is set to ledger-indent-line?

@purcell
Copy link
Member

purcell commented Apr 1, 2024

ie. what you've implemented feels the same as simply not setting indent-region-function in the first place.

@jdek
Copy link
Author

jdek commented Apr 1, 2024

Good catch, I wasn't aware that indent-region worked like that by default. Just unsetting indent-region-function seems to work for me.

Test input:

    2017-06-26 Commonplace Coffee
         Expenses:Restaurants:Coffee                 3.00   ; Grande
    Assets:Cash:Wallet                         -3.00

2017-06-26 Commonplace Coffee
       Expenses:Restaurants:Coffee                 3.00   ; Grande
    Assets:Cash:Wallet                         -3.00

          2017-06-26 Commonplace Coffee
      Assets:Cash:Wallet
        Expenses:Restaurants:Coffee                 3.00

Current master:

    2017-06-26 Commonplace Coffee
   Expenses:Restaurants:Coffee                 3.00   ; Grande
   Assets:Cash:Wallet                         -3.00

2017-06-26 Commonplace Coffee
   Expenses:Restaurants:Coffee                 3.00   ; Grande
   Assets:Cash:Wallet                         -3.00

   2017-06-26 Commonplace Coffee
   Assets:Cash:Wallet
   Expenses:Restaurants:Coffee                 3.00

With (setq-local indent-region-function nil):

2017-06-26 Commonplace Coffee
   Expenses:Restaurants:Coffee                 3.00   ; Grande
   Assets:Cash:Wallet                         -3.00

2017-06-26 Commonplace Coffee
   Expenses:Restaurants:Coffee                 3.00   ; Grande
   Assets:Cash:Wallet                         -3.00

2017-06-26 Commonplace Coffee
   Assets:Cash:Wallet
   Expenses:Restaurants:Coffee                 3.00

ledger-post-align-postings is broken somehow I guess?

@purcell
Copy link
Member

purcell commented Apr 1, 2024

ledger-post-align-postings is broken somehow I guess?

Hmmm... do we have any tests for that, perhaps? You could consider pushing a failing test to show what you'd expect.

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.

Can't indent regions
3 participants