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

Mysql support and other fixes #11

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

Conversation

sphakka
Copy link

@sphakka sphakka commented Jul 28, 2018

Added support for MySql with related options. Tested with GnuCash 3.x (warning: since v2.7, GnuCash DB must be updated).
Fixed template to pass W3C validation and avoid HTML escaping of embedded CSS.
Some more doc.

Marco Emilio Poleggi added 4 commits July 24, 2018 16:42
* Minimal config file
* Better rescueing on DB connection
* Extended host option with '[:PORT]'
* More doc

or:

$ gnucash-invoice -a mysql -h db.somewhere.net -d dbname -u user -p sekreet
Copy link
Owner

Choose a reason for hiding this comment

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

Probably it's better to change to simply accept --db argument that will be a URL like:

sqlite:///path/to/db.sqlite3
mysql://user:pass@db.somewherenet/dbname

Copy link
Owner

Choose a reason for hiding this comment

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

Not saying I insist. Just a thought,

@@ -0,0 +1,6 @@
# constants
DATE_FMT = "%Y-%m-%d".freeze # ISO
Copy link
Owner

Choose a reason for hiding this comment

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

Can you move those constants under GnuCash module? And place them inside of lib/gnucash.rb?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine if you will move it to lib/gnucash/constants.rb though or lib/gnucash/core.rb for example.

'Invoice ID', 'Customer', 'Reference', 'Opened at', 'Posted at',
'Due at'
]
puts INVOICE_LIST_HDR
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think these two should be constants.

I think we should add teminal-table gem instead and use it here:

puts Terminal::Table.new({
  :headings => ['Invoice ID', 'Customer', 'Reference', 'Opened at', 'Posted at', 'Due at']
  :rows => Invoice.all.map { |invoice| [invoice.id, ...] }
})

rescue GnuCash::NoDatabaseFound,
GnuCash::NoDatabaseConnection,
Sequel::DatabaseConnectionError => e
abort("ERROR: #{options[:db].inspect}: Can't connect to database: #{e}")
Copy link
Owner

Choose a reason for hiding this comment

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

It's better to use wording:

abort("ERROR: Can't connect to database #{options[:db].inspect}\n#{e}")

# other exceptions beyond this point are *bugs*
# rescue => e
# abort("ERROR: #{options[:db].inspect}: #{e}")

Copy link
Owner

Choose a reason for hiding this comment

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

Just remove this then :D

head
meta charset="utf-8"
title = "#{invoice.id} (#{invoice.posted? ? invoice.posted_at : "DRAFT"})"
style type="text/css" = embedded_asset('main.css')
/ CSS must not be HTML-escaped
Copy link
Owner

Choose a reason for hiding this comment

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

wrong comment indentation

Copy link
Owner

Choose a reason for hiding this comment

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

in fact there's no need in this comment at all :D

@ixti
Copy link
Owner

ixti commented Aug 1, 2018

I'm pretty happy to merge this after some comments will be addressed

@sphakka
Copy link
Author

sphakka commented Aug 2, 2018

Thanks for the comment. I'll review and correct!

@ixti
Copy link
Owner

ixti commented Aug 2, 2018

Thank you!

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