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

allow multple comments in a line for the new compact syntax #143

Merged
merged 2 commits into from
Mar 2, 2017
Merged

allow multple comments in a line for the new compact syntax #143

merged 2 commits into from
Mar 2, 2017

Conversation

gerdreiss
Copy link
Contributor

  • extended parser for compact transaction syntax to allow multple comments
  • added test case for multiple comments in a line

@hrj
Copy link
Owner

hrj commented Mar 1, 2017

@gerdreiss I think you are on the right track, but this change still affects multi-line-transactions.

Test case:

test143.ledger:

2017/march/30
  ; comment 1 ; comment 2
  bank 200
  cash

test143.conf

inputs += test143.ledger

exports += {
  format = xml
  type = journal
}

reports = []

gives:

[info]   ; comment 1 ; comment 2
[info]               ^
[info] Input error: Couldn't parse input

@hrj
Copy link
Owner

hrj commented Mar 1, 2017

Forgot to mention, I expect it to give the same output as current code. That is:

[info] <abandon version="Base: 0.4.0 [2017-03-01 18:39:24.660];CLI: 0.4.0 [2017-03-01 18:39:24.631];">
[info]   <journal>
[info]     <transactions>
[info]       <txn date="2017-03-30">
[info]         <comment> comment 1 ; comment 2</comment>
[info]         <post delta="200" name="bank"></post>
[info]         <post delta="-200" name="cash"></post>
[info]       </txn>
[info]     </transactions>
[info]   </journal>
[info] </abandon>

([info] is coming from sbt)

@gerdreiss
Copy link
Contributor Author

gerdreiss commented Mar 1, 2017

Actually, my change does not affect the multiline transactions, I thought, because I haven't changed the parsing of those (Edit: sorry, it does, of course, as I don't allow semicolons there...). I understood that we didn't want that from what you've said in your issue comment:

"for now, we will make the syntax change only for single-line-notation. Otherwise it will affect older input files."

I must have misunderstood that. I thought that we want to support multiple comments in one line for compact transaction syntax only. So, do we want to handle the multiple comments there as one? At least that is how I understand the XML output...

Edit 2: Sorry, I think I understand my mistake :) I'll look into it...

@hrj
Copy link
Owner

hrj commented Mar 1, 2017

Regarding your edit: Thanks for looking into it!

@gerdreiss
Copy link
Contributor Author

@hrj Just to avoid further misunderstandings and mistakes ;)

I suppose that this:

2017/march/30
  ; comment 1 ; comment 2
  bank 200  ; comment 4 ; comment 3
  cash

should lead to this:

2017/march/30
  ; comment 1 ; comment 2 ; comment 3
  bank 200  ; comment 4
  cash

Correct?

@hrj
Copy link
Owner

hrj commented Mar 1, 2017

Nope :)

Since it's multi-line syntax, it shouldn't change behavior from existing code.

Only single-line syntax (with the dot in the front) needs to change.

Glad you asked! If it's still confusing, we could first try to add tests and then change the code (test driven development).


I am nearing my sleep time here. I can look into it tomorrow, if you don't beat me to it.

@hrj
Copy link
Owner

hrj commented Mar 2, 2017

@gerdreiss

I have added two tests for comments in 3dc10ea

If you rebase this branch and push again, I think this PR will be verified automatically by Travis.

Gerd Reiss added 2 commits March 2, 2017 17:57
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.351% when pulling 3bb9bcc on gerdreiss:i97-multiple-comments-2 into 6a0d61b on hrj:master.

@hrj hrj merged commit 5f65742 into hrj:master Mar 2, 2017
@gerdreiss gerdreiss deleted the i97-multiple-comments-2 branch March 2, 2017 17:43
@hrj
Copy link
Owner

hrj commented Mar 2, 2017

@gerdreiss Another tip. If you add "closes #issue-number" to the PR description, the issue will be closed automatically when the PR is merged.

@gerdreiss
Copy link
Contributor Author

@hrj good to know. my next PR will have that description 👍

@hrj hrj modified the milestones: Next release, 0.5.0 May 10, 2017
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.

3 participants