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

#97 Allow multiple comment entries in single line notation #138

Closed
wants to merge 2 commits into from
Closed

#97 Allow multiple comment entries in single line notation #138

wants to merge 2 commits into from

Conversation

gerdreiss
Copy link
Contributor

@gerdreiss gerdreiss commented Feb 22, 2017

Hi @hrj , that should be it. Please, let me know if there is anything I could do better, or should do differently.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.324% when pulling 42ed45d on gerdreiss:i97-multiple-comments into f01de5c on hrj:master.

@hrj
Copy link
Owner

hrj commented Feb 23, 2017

Hi @gerdreiss. Thanks for looking into it.

I had imagined the extra comments following the post comment would become transaction comments, not post comments. I should have clarified that in the issue.

What you have done is fine too, from the code perspective, but it changes the export format and behaviour of subsequent tools that consume the exports. For example, I have a reporting tool which consumes the XML exported from abandon. There might be others too who are using abandon this way. These would require a rewrite if the format changes.

It would be ideal if we could move these extra comments to transaction level.

Again, sorry, I should have expressed this in the issue!

@gerdreiss
Copy link
Contributor Author

HI @hrj , no problem at all, I'll gladly move the comments to transaction level :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.324% when pulling b40db12 on gerdreiss:i97-multiple-comments into f01de5c on hrj:master.

@gerdreiss
Copy link
Contributor Author

gerdreiss commented Feb 23, 2017

Hi @hrj , I've moved the extra post comments to transaction level (hopefully now like you imagined ;-)), but the commit history got a little ugly. I'd like to make a new pull request with one pretty commit (from a new branch). What do you think?

@hrj
Copy link
Owner

hrj commented Feb 23, 2017

@gerdreiss Sure, you can create a new Pull request.

Or if you are familiar with git rebasing, you can rebase and push again to this branch. (Not recommended if you are new to git)

…le comments after a post, only the first one remains with the post, the rest goes to the transaction
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.324% when pulling 80a84f9 on gerdreiss:i97-multiple-comments into f01de5c on hrj:master.

@gerdreiss
Copy link
Contributor Author

@hrj Rebase and push was a good idea - now there is just one small commit :)

@hrj
Copy link
Owner

hrj commented Feb 27, 2017

@gerdreiss Can you change it so that the new syntax only applies to single-line-notation. For multi-line notation, we will maintain status quo for now.

Sorry for chickening out :)

@gerdreiss
Copy link
Contributor Author

@hrj , sorry for being a bit dense ;-) but I'm not quite sure I understand the change you're requesting...

@hrj
Copy link
Owner

hrj commented Feb 28, 2017

@gerdreiss

There are two syntaxes for transactions. The older and fuller syntax:

date
  ; txn comment
  account     amount    ; post comment
  account     amount    ; post comment

And the newer, shorter, single line syntax:

. date   account    amount       ; post comment

If I understand right, this PR changes how post comments are parsed, for both syntaxes. However, I would like to make this change only for the shorter, single-line syntax.

The reason is that the older syntax has been around for years and I don't want to break it. The newer syntax we can afford to break a little, especially since the newer syntax has no way to enter a txn level comment.

- added test case for multiple comments for new syntax.
@gerdreiss
Copy link
Contributor Author

@hrj I hope I understood the required changes correctly. see my last commit.

@gerdreiss
Copy link
Contributor Author

gerdreiss commented Feb 28, 2017

@hrj Something went wrong with the build on travis - some dependencies could not be resolved. I'll take that as an excuse to create a new, clean PR :)

Edit: oh, there are issues due to S3 outage in AWS...

@gerdreiss
Copy link
Contributor Author

@hrj So I've created a new PR with changes for the compact transaction syntax only and a clean commit history. I think we can close this one, if you agree

@hrj
Copy link
Owner

hrj commented Mar 1, 2017

@gerdreiss Great; will review the other PR and close this one. Thanks!

@hrj hrj closed this Mar 1, 2017
@gerdreiss gerdreiss deleted the i97-multiple-comments branch March 2, 2017 17:31
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