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

Add support for parsing accounts #13

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

Conversation

davidsklar
Copy link

This adds support for parsing the !Account block in a QIF file.

Because the block could occur anywhere in the file (in the files I am working with they are at the very end) I made it so read_record would return it when found.

If it is undesirable that a record is no longer always a transaction, instead the account block could be looked for when the file is open (similar to the headers) but this could be very inefficient if the whole file has to be scanned before it is found.

@jonbartlett
Copy link

Does the PR just enable reading of the !Account block? I need to write this to the QIF file.

@davidsklar
Copy link
Author

Yup, it just enables reading the !Account block.

@jonbartlett
Copy link

Ok thought so. I have worked on a PR that implements the write and also added a few tests.

How to proceed from here?

Shall I initiate a PR to your forked version? and you initiate a new PR on the base repo?

Or should I initiate a PR on the base repo directly?

@davidsklar
Copy link
Author

Whatever you want -- I'm happy to merge your stuff into my PR if you want.

@jonbartlett
Copy link

Done.

davidsklar#1

Added ability to write Account transaction and additional tests.
@@ -61,11 +62,17 @@ def initialize(io, type = 'Bank', format = 'dd/mm/yyyy')

# Add a transaction for writing
def <<(transaction)
@transactions << transaction
case transaction.class.to_s
Copy link
Owner

Choose a reason for hiding this comment

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

I think this part of the PR needs some work. If there can only be one account then it seems it should not be set using the add transaction method here. Nor should it use an array instance variable.

Copy link

Choose a reason for hiding this comment

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

Yes. It would be better if it raised an exception rather than silently handling more than one account.
On that note should 'write' fail if there is no account?

Im not really sure how these files work im just going off the code and comments.

Choose a reason for hiding this comment

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

This did cross my mind when I wrote this. According to the QIF spec:

The account header !Account is used in two places-at the start of an account list and the start of a list of transactions to specify to which account they belong.
via http://www.respmech.com/mym2qifw/qif_new.htm

My take on the first use "at the start of an account list" is for when the QIF file contains an account structure and no transaction data, therefore not relevant.

In the second use case, at the "start of a list of transactions to specify to which account they belong", the !Account block is used to describe the subsequent transactions. Therefore the subsequent Transactions belong to the Account. I have seen multiple Account blocks in a QIF file where the transactions appear sequentially for multiple accounts. Think of the Account section being like a header for the transaction records.

To implement the above would need quite a re-write to both Account and Transaction classes and associated reader and writer methods.

For what I am working on I only need one Account per file but will look into making the other changes.

Copy link

Choose a reason for hiding this comment

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

@davidsklar did say that his !Account was found at the end of the file. But if !Account is at the start of a transaction block something like this should work without too much of a refactor.

@accounts = {}

def <<(transaction)
  case transaction.class.name 
  when 'Qif::Account'     then @account = transaction.to_s
  when 'Qif::Transaction' 
    @accounts[@account] ||= []
    @accounts[@account] << transaction
  else raise "Cannot handle #{transaction.class.name.inspect}"
  end
end

def write
  @accounts.each do |account,transactions|
    write_record account
    write_header
    transactions.each {|t| write_transaction }
  end
end

Copy link
Author

Choose a reason for hiding this comment

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

I have also run across .QIF files with multiple account blocks. For example, in an export from Moneydance, there are a number of !Account blocks at the beginning of the file, one after another, describing/declaring each account, and then the !Account blocks recur later, followed by a !Type statement and then a bunch of transactions for that account.

Copy link
Owner

Choose a reason for hiding this comment

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

I think that we should support multiple accounts then, if someone has the time to update this PR?

Choose a reason for hiding this comment

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

Yes I agree now. The QIF format seems very loose so good to let the developer define the format as per their use case. So we now have multiple Accounts and Transactions. No need to create any relationship between them in this gem.

I have just submitted a PR to @davidsklar that handles writing multiple accounts. davidsklar#3

@davidsklar Can you merge please.

@jemmyw
Copy link
Owner

jemmyw commented Jul 7, 2015

Thanks for the PR, I think this looks good but I would like the writer part to be tightened up a little.

Jon Bartlett and others added 2 commits September 3, 2015 17:03
@jonbartlett
Copy link

Hi, where did we get with this? I see my changes got merged but didn't go anywhere from here.

davidsklar@fb0e2db

@jemmyw
Copy link
Owner

jemmyw commented Mar 14, 2016

Hi @jonbartlett and @davidsklar , would either or both of you like to take this repo and gem off my hands? I'm no longer using it so I've not got much motivation for fixing up this sort of thing.

@jonbartlett
Copy link

Hi @jemmyw yes I would be interested as I have plans to use this with the additional functionality.

@jemmyw
Copy link
Owner

jemmyw commented Mar 16, 2016

@jonbartlett I just need an email address to transfer the gem ownership to on rubygems.org. Either post something here that I can use, or email me using my github username at gmail

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.

None yet

4 participants