Implement proper logging and stop using puts

- No polluted output when running tests!
- Possibility to add debug output that will only show when setting
  the environment variable DEBUG=1
- Maybe this won't do so well with Thor and TTY (might want to use
  methods such as `say`)

.rubocop.yml

@@ -21,6 +21,11 @@ Metrics/AbcSize:
   Exclude:
     - 'test/**/*_test.rb'
 
+Metrics/ClassLength:
+  Max: 150
+  Exclude:
+    - 'test/**/*_test.rb'
+
 Metrics/MethodLength:
   Max: 20
   Exclude:

README.md

@@ -60,9 +60,6 @@ After checking out the repo, run `bundle install` to install dependencies. Then,
 - Handle the Amazon account differently (use account name as payee instead of content?)
 - Implement CLI to setup config.
   - Save/update session_id so browser is only needed once.
-- Improve logging/output.
-  - Debug logging.
-  - Prevent logging to STDOUT when running tests.
 - Get rid of `envchain`?
   - Store config/credentials in `~/.config/`?
   - Encrypt config, use Keyring or other OS-level secure storage?

lib/mfynab/cli.rb

@@ -16,7 +16,7 @@ def initialize(argv)
   end
 
   def start
-    puts "Running..."
+    logger.info("Running...")
 
     Dir.mktmpdir("mfynab") do |save_path|
       money_forward.download_csv(
@@ -25,10 +25,12 @@ def start
         months: config["months_to_sync"],
       )
 
-      data = MFYNAB::MoneyForwardData.new
+      data = MFYNAB::MoneyForwardData.new(logger: logger)
       data.read_all_csv(save_path)
       ynab_transaction_importer.run(data.to_h)
     end
+
+    logger.info("Done!")
   end
 
   private
@@ -47,11 +49,12 @@ def ynab_transaction_importer
         config["ynab_access_token"],
         config["ynab_budget"],
         config["accounts"],
+        logger: logger,
       )
     end
 
     def money_forward
-      @_money_forward ||= MFYNAB::MoneyForward.new
+      @_money_forward ||= MFYNAB::MoneyForward.new(logger: logger)
     end
 
     def config_file
@@ -72,4 +75,16 @@ def config
           "moneyforward_password" => ENV.fetch("MONEYFORWARD_PASSWORD"),
         )
     end
+
+    def logger
+      @_logger ||= Logger.new($stdout, level: logger_level)
+    end
+
+    def logger_level
+      if ENV.fetch("DEBUG", nil)
+        Logger::DEBUG
+      else
+        Logger::INFO
+      end
+    end
 end

lib/mfynab/money_forward.rb

@@ -11,8 +11,9 @@ class MoneyForward
     USER_AGENT = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) " \
                  "AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36"
 
-    def initialize(base_url: DEFAULT_BASE_URL)
+    def initialize(logger:, base_url: DEFAULT_BASE_URL)
       @base_url = URI(base_url)
+      @logger = logger
     end
 
     def get_session_id(username:, password:)
@@ -39,7 +40,7 @@ def download_csv(session_id:, path:, months:)
         months.times do
           date_string = month.strftime("%Y-%m")
 
-          puts "Downloading CSV for #{date_string}"
+          logger.info("Downloading CSV for #{date_string}")
 
           # FIXME: I don't really need to save the CSV files to disk anymore.
           # Maybe just return parsed CSV data?
@@ -74,7 +75,7 @@ def download_csv_string(date:, session_id:)
 
     private
 
-      attr_reader :base_url
+      attr_reader :base_url, :logger
 
       def with_ferrum
         browser = Ferrum::Browser.new(timeout: 30, headless: !ENV.key?("NO_HEADLESS"))
@@ -85,7 +86,7 @@ def with_ferrum
         yield browser
       rescue StandardError
         browser.screenshot(path: "screenshot.png")
-        puts "An error occurred and a screenshot was saved to ./screenshot.png"
+        logger.error("An error occurred and a screenshot was saved to ./screenshot.png")
         raise
       ensure
         browser&.quit

lib/mfynab/money_forward_data.rb

@@ -17,8 +17,9 @@ class MoneyForwardData
       id: "ID",
     }.freeze
 
-    def initialize
+    def initialize(logger:)
       @transactions = {}
+      @logger = logger
     end
 
     def read_all_csv(path)
@@ -28,7 +29,7 @@ def read_all_csv(path)
     end
 
     def read_csv(csv_file)
-      puts "Reading #{csv_file}"
+      logger.info("Reading #{csv_file}")
       CSV.foreach(
         csv_file,
         headers: true,
@@ -46,7 +47,7 @@ def to_h
 
     private
 
-      attr_reader :transactions
+      attr_reader :transactions, :logger
 
       def csv_header_converters
         lambda do |header|

lib/mfynab/ynab_transaction_importer.rb

@@ -4,13 +4,14 @@
 
 module MFYNAB
   class YnabTransactionImporter
-    def initialize(api_key, budget_name, account_mappings)
+    def initialize(api_key, budget_name, account_mappings, logger:)
       @api_key = api_key
       @budget_name = budget_name
       @account_mappings = account_mappings
+      @logger = logger
     end
 
-    def run(mf_transactions)
+    def run(mf_transactions) # rubocop:disable Metrics/AbcSize
       mf_transactions.map do |mf_account, mf_account_transactions|
         account = ynab_account_for_mf_account(mf_account)
         next unless account
@@ -20,17 +21,17 @@ def run(mf_transactions)
         end
 
         begin
-          puts "Importing #{transactions.size} transactions for #{account.name}"
+          logger.info("Importing #{transactions.size} transactions for #{account.name}")
           ynab_transactions_api.create_transaction(budget.id, transactions: transactions)
         rescue StandardError => e
-          puts "Error importing transactions for #{budget.name}. #{e} : #{e.detail}"
+          logger.error("Error importing transactions for #{budget.name}. #{e} : #{e.detail}")
         end
       end
     end
 
     private
 
-      attr_reader :api_key, :budget_name, :account_mappings
+      attr_reader :api_key, :budget_name, :account_mappings, :logger
 
       def convert_mf_transaction(row, account)
         {
@@ -106,7 +107,7 @@ def ynab_account_for_mf_account(mf_account_name)
         end
 
         unless matching_mapping
-          puts "Debug: no mapping for MoneyForward account #{mf_account_name}. Skipping..."
+          logger.debug("Debug: no mapping for MoneyForward account #{mf_account_name}. Skipping...")
           return
         end
 

test/mfynab/money_forward_data_test.rb

@@ -24,7 +24,7 @@ def test_read_all_csv_reads_all_csv_in_directory_and_groups_by_account
         csv1.save_to(dir)
         csv2.save_to(dir)
 
-        data = MoneyForwardData.new
+        data = MoneyForwardData.new(logger: null_logger)
         data.read_all_csv(dir)
 
         transactions = data.to_h
@@ -65,7 +65,7 @@ def test_read_csv_reads_csv_file_and_converts_headers
       Tempfile.open("mfynabcsv") do |file|
         csv.save_to(file)
 
-        data = MoneyForwardData.new
+        data = MoneyForwardData.new(logger: null_logger)
         data.read_csv(file)
 
         expected_transactions = {
@@ -86,5 +86,11 @@ def test_read_csv_reads_csv_file_and_converts_headers
         assert_equal expected_transactions, data.to_h
       end
     end
+
+    private
+
+      def null_logger
+        Logger.new(File::NULL)
+      end
   end
 end

test/mfynab/money_forward_test.rb

@@ -10,7 +10,10 @@ module MFYNAB
   class MoneyForwardTest < Minitest::Test
     def test_get_session_id_raises_if_wrong_credentials
       while_running_fake_money_forward_app do |host, port|
-        money_forward = MoneyForward.new(base_url: "http://#{host}:#{port}")
+        money_forward = MoneyForward.new(
+          base_url: "http://#{host}:#{port}",
+          logger: null_logger,
+        )
 
         assert_raises(RuntimeError, "Login failed") do
           money_forward.get_session_id(
@@ -25,6 +28,7 @@ def test_get_session_id_happy_path
       while_running_fake_money_forward_app do |host, port|
         session_id = MoneyForward.new(
           base_url: "http://#{host}:#{port}",
+          logger: null_logger,
         ).get_session_id(
           username: "david@example.com",
           password: "Passw0rd!",
@@ -45,7 +49,7 @@ def test_download_csv_downloads_csv_by_passing_a_cookie_and_converts_data_to_utf
       end
 
       Dir.mktmpdir do |tmpdir|
-        MoneyForward.new.download_csv(
+        MoneyForward.new(logger: null_logger).download_csv(
           session_id: session_id,
           path: tmpdir,
           months: 3,
@@ -120,5 +124,9 @@ def responsive?(webapp_thread, host, port)
       rescue SystemCallError, Net::ReadTimeout, OpenSSL::SSL::SSLError
         false
       end
+
+      def null_logger
+        Logger.new(File::NULL)
+      end
   end
 end

test/mfynab/ynab_transaction_importer_test.rb

@@ -26,6 +26,7 @@ def test_with_no_transactions
         "dummy_api_key",
         ynab_budget[:name],
         [{ "money_forward_name" => mf_account_name, "ynab_name" => ynab_account[:name] }],
+        logger: null_logger,
       )
       importer.run({})
 
@@ -50,6 +51,7 @@ def test_when_money_forward_account_not_mapped
         "dummy_api_key",
         ynab_budget[:name],
         [],
+        logger: null_logger,
       )
 
       importer.run({ # Should not raise
@@ -97,6 +99,7 @@ def test_happy_path
         "dummy_api_key",
         ynab_budget[:name],
         [{ "money_forward_name" => mf_account_name, "ynab_name" => ynab_account[:name] }],
+        logger: null_logger,
       )
       importer.run({
         mf_account_name => [{
@@ -114,5 +117,11 @@ def test_happy_path
 
       assert_requested(expected_transactions_request)
     end
+
+    private
+
+      def null_logger
+        Logger.new(File::NULL)
+      end
   end
 end