استيراد من قاعدة بيانات تطبيق بلوكوينز المحاسبي - #1

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (1)

108-151: Consider separating error printing from validation logic.

The function currently prints error messages directly (line 120), which reduces its reusability. Consider returning status information and letting callers handle error messaging, or accepting an optional callback for error handling.

That said, for a script-level utility, this approach is acceptable.

Note on static analysis hints:

• Line 139: The immediate return True is clear; moving to an else block is purely stylistic.
• Line 145: Catching Exception is intentional here to handle any unexpected errors (permissions, I/O issues, etc.) and return False. This is appropriate for a validation function.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (1)

455-457: Remove redundant error message.

The error message at line 456 is redundant with the one already printed by is_valid_sqlite_db at line 123 when verbose=True (set at line 450). Additionally, for non-existent files, users will see two error messages, which is confusing.

Apply this diff to remove the redundancy:

     if not is_valid_sqlite_db(args.db_file):
-        print(f"error: {args.db_file} is invalid sqlite3 database")
         sys.exit(1)

playground/bluecoins.py (2)

193-201: Use pythonic boolean checks in assertions.

Replace explicit comparisons to True and False with idiomatic Python boolean checks for better readability.

Apply this diff:

-        assert is_valid_sqlite_db('non_existent.db') == False, "Test 1 Failed: Non-existent file."
+        assert not is_valid_sqlite_db('non_existent.db'), "Test 1 Failed: Non-existent file."

-        assert is_valid_sqlite_db(valid_db_path) == True, "Test 2 Failed: Valid SQLite DB."
+        assert is_valid_sqlite_db(valid_db_path), "Test 2 Failed: Valid SQLite DB."

-        assert is_valid_sqlite_db(invalid_file_path) == False, "Test 3 Failed: Invalid text file."
+        assert not is_valid_sqlite_db(invalid_file_path), "Test 3 Failed: Invalid text file."

---

447-448: Consider making tests optional.

Running tests on every script execution adds overhead. Consider moving tests behind a --test flag or to a separate test file that can be run independently.

Example implementation:

if __name__ == "__main__":
    parser = argparse.ArgumentParser(description="Process Bluecoins database and export data to CSV.")
    parser.add_argument("db_file", help="Path to the Bluecoins database file (.fydb)")
    parser.add_argument("--test", action="store_true", help="Run tests before processing")
    args = parser.parse_args()
    
    if args.test:
        test_add_millisecond_and_format()
        test_is_valid_sqlite_db()
        print("All tests passed!")
    
    verbose = True
    if not is_valid_sqlite_db(args.db_file):
        sys.exit(1)
    process_bluecoins_data(args.db_file)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (2)

133-137: Typo fix confirmed.

“thr.eading” typo is corrected to “threading”. LGTM.

---

459-461: Remove duplicate error output; let caller print once.

is_valid_sqlite_db no longer prints (see earlier suggestion). Keep a single, clear message here and send it to stderr.

-    if not is_valid_sqlite_db(args.db_file):
-        print(f"error: {args.db_file} is invalid sqlite3 database")
-        sys.exit(1)
+    if not is_valid_sqlite_db(args.db_file):
+        print(f"error: {args.db_file} is invalid sqlite3 database", file=sys.stderr)
+        sys.exit(1)

playground/bluecoins.py (7)

64-87: Tests encode microsecond behavior; align with chosen unit.

If you adopt Option A (microseconds), rename the test and its parameter names; if Option B (milliseconds), update expected outputs (+0.001s per increment).

Example for Option A:

-def test_add_millisecond_and_format():
+def test_add_microsecond_and_format():
@@
-        ("2023-10-27 10:30:45.123455", "2023-10-27 10:30:45.123457", 2), # test with extra_ms=2
-        ("2023-10-27 10:30:45", "2023-10-27 10:30:45.000002", 2), # test with extra_ms=2, and no initial ms
+        ("2023-10-27 10:30:45.123455", "2023-10-27 10:30:45.123457", 2), # extra_us=2
+        ("2023-10-27 10:30:45", "2023-10-27 10:30:45.000002", 2),       # extra_us=2
@@
-        extra_ms = test_case[2] if len(test_case) > 2 else 1
-        actual_output = add_millisecond_and_format(input_str, extra_ms)
+        extra_us = test_case[2] if len(test_case) > 2 else 1
+        actual_output = add_microsecond_and_format(input_str, extra_us)
-        assert actual_output == expected_output, f"Test failed for input: {input_str}, actual: {actual_output}, expected: {expected_output}, extra_ms: {extra_ms}"
+        assert actual_output == expected_output, f"Test failed for input: {input_str}, actual: {actual_output}, expected: {expected_output}, extra_us: {extra_us}"

Also update Line 451 to call the new test if renamed.

---

124-159: Validator should not print; open DB read-only; narrow exceptions.

• Return a boolean only (no printing) to avoid duplicated/conflicting messages.
• Use read-only URI to prevent accidental writes.
• Restructure try/except/else/finally and narrow exception types.

-    # 1. Check if the file exists first for an early exit
-    if not os.path.exists(db_path):
-        if verbose:
-            print(f"error: {db_path} doesn't exist")
-        return False
+    # 1. Early exit if the path does not exist
+    if not os.path.exists(db_path):
+        return False
@@
-        conn = sqlite3.connect(db_path)
+        # Open read-only to avoid side effects; requires uri=True
+        conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True, timeout=1.0)
@@
-        return True
-
-    except sqlite3.DatabaseError:
-        # Catches specific errors indicating the file is not a valid SQLite format
-        # (e.g., "file is not a database")
-        return False
-    except Exception:
-        # Catch any other unexpected errors (like permission denied, etc.)
-        return False
+    except (sqlite3.DatabaseError, sqlite3.OperationalError, PermissionError, OSError):
+        # Not a DB, cannot open, or no permission -> invalid for our purposes
+        return False
+    else:
+        return True
     finally:
         # 3. Ensure the connection is closed
         if conn:
             conn.close()

Based on static analysis hints.

---

169-189: Narrow broad exception handlers in tests; fix E712 comparisons.

• Replace bare except Exception with specific exceptions.
• Use not ... instead of == False and direct truth check instead of == True.

-    except Exception as e:
+    except sqlite3.Error as e:
         print(f"Setup Error: Failed to create valid DB file. Exiting. {e}")
         return
@@
-    except Exception as e:
+    except OSError as e:
         print(f"Setup Error: Failed to create invalid text file. Exiting. {e}")
         return
@@
-        assert is_valid_sqlite_db('non_existent.db') == False, "Test 1 Failed: Non-existent file."
+        assert not is_valid_sqlite_db('non_existent.db'), "Test 1 Failed: Non-existent file."
@@
-        assert is_valid_sqlite_db(valid_db_path) == True, "Test 2 Failed: Valid SQLite DB."
+        assert is_valid_sqlite_db(valid_db_path), "Test 2 Failed: Valid SQLite DB."
@@
-        assert is_valid_sqlite_db(invalid_file_path) == False, "Test 3 Failed: Invalid text file."
+        assert not is_valid_sqlite_db(invalid_file_path), "Test 3 Failed: Invalid text file."
@@
-    except Exception as e:
-        print(f"An unexpected error occurred during testing: {e}")
+    # Let unexpected errors surface; if you prefer to log, narrow types explicitly.

Based on static analysis hints.

Also applies to: 210-214

---

343-347: Parameterize SQL to avoid interpolation risks.

Even if values originate from the DB, prefer placeholders; safer and consistent.

-                labels = cursor.execute(f"""
-                    SELECT labelName
-                    FROM LABELSTABLE
-                    WHERE transactionIDLabels = {id1};
-                """).fetchall()
+                labels = cursor.execute("""
+                    SELECT labelName
+                    FROM LABELSTABLE
+                    WHERE transactionIDLabels = ?;
+                """, (id1,)).fetchall()

---

354-355: Only append microseconds when missing.

Avoid producing strings like “…%f.000000” if the DB already stores fractional seconds.

-                    date1 + ".000000",
+                    date1 if "." in date1 else (date1 + ".000000"),

---

312-315: Avoid interactive prompt and sys.exit inside library logic; add a --yes flag and move exits to main.

• Remove input() and sys.exit() from process_bluecoins_data; return instead.
• Add --yes to skip confirmation.
• Print errors to stderr and avoid duplicate messages (see next comment).

-        user_input = input("Type 'Y' to continue or anything for exit: ")
-        if user_input.capitalize() != 'Y':
-            sys.exit(0)
+        # Confirmation moved to __main__
         print("Processing...")
@@
-        print('OK')
-        sys.exit(0)
+        print('OK')
+        return 0

And in __main__:

-    parser = argparse.ArgumentParser(description="Process Bluecoins database and export data to CSV.")
+    parser = argparse.ArgumentParser(description="Process Bluecoins database and export data to CSV.")
     parser.add_argument("db_file", help="Path to the Bluecoins database file (.fydb)")
+    parser.add_argument("-y", "--yes", action="store_true", help="Proceed without confirmation prompt")
     args = parser.parse_args()
@@
-    if not is_valid_sqlite_db(args.db_file):
-        print(f"error: {args.db_file} is invalid sqlite3 database")
-        sys.exit(1)
-    process_bluecoins_data(args.db_file)
+    if not is_valid_sqlite_db(args.db_file):
+        print(f"error: {args.db_file} is invalid sqlite3 database", file=sys.stderr)
+        sys.exit(1)
+    if not args.yes:
+        user_input = input("Type 'Y' to continue or anything to exit: ")
+        if user_input.strip().upper() != 'Y':
+            sys.exit(0)
+    code = process_bluecoins_data(args.db_file)
+    sys.exit(code if isinstance(code, int) else 0)

Also applies to: 441-442, 449-461

---

451-455: Don’t run tests on every CLI invocation; gate behind a flag.

Surprising for users; add --self-test to run tests explicitly.

-    # Run the tests
-    test_add_millisecond_and_format()
-    test_is_valid_sqlite_db()
-    # Continue the rest
-    verbose = True
+    parser = argparse.ArgumentParser(description="Process Bluecoins database and export data to CSV.")
+    parser.add_argument("--self-test", action="store_true", help="Run module self-tests and exit")
+    parser.add_argument("-v", "--verbose", action="store_true", help="Enable verbose output")
+    parser.add_argument("db_file", nargs="?", help="Path to the Bluecoins database file (.fydb)")
+    args, _ = parser.parse_known_args()
+    verbose = bool(args.verbose)
+    if args.self_test:
+        # Keep names in sync with the chosen Option A/B above
+        test_add_millisecond_and_format()
+        test_is_valid_sqlite_db()
+        sys.exit(0)
+    # Rebuild argparse or reuse as shown in previous diff when integrating flags together.

Note: Integrate with the previous argparse diff to avoid duplicate parser creation.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (4)

56-56: Minor grammar fix in comment.

The comment uses past tense "added" but should use present tense "adds" for consistency with code documentation style.

Apply this diff:

-            datetime_str += ".000000" #added microseconds if not present
+            datetime_str += ".000000"  # adds microseconds if not present

Note: Also added a space after # for PEP 8 compliance.

---

197-205: Improve boolean comparison style.

PEP 8 recommends using not for False checks and direct truthiness for True checks rather than explicit equality comparisons.

Apply this diff:

         # Test Case 1: Non-existent file
         # Expect False for a file that does not exist
-        assert is_valid_sqlite_db('non_existent.db') == False, "Test 1 Failed: Non-existent file."
+        assert not is_valid_sqlite_db('non_existent.db'), "Test 1 Failed: Non-existent file."

         # Test Case 2: Known valid database file
         # Expect True for the correctly created SQLite file
-        assert is_valid_sqlite_db(valid_db_path) == True, "Test 2 Failed: Valid SQLite DB."
+        assert is_valid_sqlite_db(valid_db_path), "Test 2 Failed: Valid SQLite DB."

         # Test Case 3: Invalid text file (exists but is not a DB)
         # Expect False for the simple text file
-        assert is_valid_sqlite_db(invalid_file_path) == False, "Test 3 Failed: Invalid text file."
+        assert not is_valid_sqlite_db(invalid_file_path), "Test 3 Failed: Invalid text file."

---

342-342: Remove unnecessary f-string prefix.

The f-string has no placeholders, so the f prefix is not needed.

Apply this diff:

-            records = cursor.execute(f"""
+            records = cursor.execute("""

---

459-461: Error message is misleading for non-existent files.

When the file doesn't exist, the error message "is invalid sqlite3 database" is misleading. The issue isn't that it's invalid, but that it doesn't exist. The user will see two messages:

1. "error: {path} doesn't exist" (from is_valid_sqlite_db)
2. "error: {path} is invalid sqlite3 database" (from this line)

Consider making the error message more generic or handling non-existence separately.

Apply this diff:

     if not is_valid_sqlite_db(args.db_file):
-        print(f"error: {args.db_file} is invalid sqlite3 database")
+        print(f"error: {args.db_file} is not a valid sqlite3 database")
         sys.exit(1)

The wording "is not a valid" is more generic and covers both non-existence and corruption cases without being misleading.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (1)

451-479: Fix CLI flow: tests always run, -v ignored, missing db_file can crash.

• Tests execute on every run (unconditional), not only with --self-test.
• verbose is forced to True, ignoring -v/--verbose.
• Running with flags only (e.g., -v) and no db_file leads to a crash in is_valid_sqlite_db(None).
• Also avoid duplicating error messages with validator (see related prior feedback).

Apply:

 if __name__ == "__main__":
-    verbose = True
     parser = argparse.ArgumentParser(description="Process Bluecoins database and export data to CSV.")
     parser.add_argument("--self-test", action="store_true", help="Run module self-tests and exit")
     parser.add_argument("-v", "--verbose", action="store_true", help="Enable verbose output")
     parser.add_argument("db_file", nargs="?", help="Path to the Bluecoins database file (.fydb)")
     parser.add_argument("-y", "--yes", action="store_true", help="Proceed without confirmation prompt")
     args = parser.parse_args()
 
-    # sys.argv is the list of command-line arguments.
-    # sys.argv[0] is the script name itself, so we check if the list has only one item.
-    if len(sys.argv) == 1:
-        print("🚨 No arguments provided.")
-        parser.print_help(sys.stderr) # Print help message to standard error stream (optional but common practice)
-        sys.exit(1) # Exit the script with a non-zero status code (convention for failure)
+    # honor verbosity flag
+    verbose = args.verbose
+
+    # self-tests only when requested
+    if args.self_test:
+        debug = True
+        test_add_microseconds_and_format()
+        test_is_valid_sqlite_db()
+        sys.exit(0)
+
+    # require db_file when not running self-tests
+    if not args.db_file:
+        parser.error("db_file is required unless --self-test")
 
-    # Run the tests
-    if args.self_test:
-        debug = True
-    test_add_microseconds_and_format()
-    test_is_valid_sqlite_db()
-    if args.self_test:
-        sys.exit(0)
     if not is_valid_sqlite_db(args.db_file):
-        print(f"error: {args.db_file} is invalid sqlite3 database")
+        print(f"error: {args.db_file} is invalid sqlite3 database")
         sys.exit(1)
     yes = args.yes
     code = process_bluecoins_data(args.db_file)
     sys.exit(code if isinstance(code, int) else 0)

playground/bluecoins.py (5)

115-160: Make DB validator pure, read‑only, and narrow exceptions.

Avoid prints inside a validator, open DB read‑only, and replace blind except Exception. This also de‑duplicates error messaging from __main__.

 def is_valid_sqlite_db(db_path: str) -> bool:
@@
-    if not os.path.exists(db_path):
-        if verbose:
-            print(f"error: {db_path} doesn't exist")
-        return False
+    if not os.path.exists(db_path):
+        return False
@@
-        conn = sqlite3.connect(db_path)
+        # open read-only; prevents creating files and journaling on probes
+        conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)
@@
-    except sqlite3.DatabaseError:
-        # Catches specific errors indicating the file is not a valid SQLite format
-        # (e.g., "file is not a database")
-        return False
-    except Exception:
-        # Catch any other unexpected errors (like permission denied, etc.)
-        return False
+    except (sqlite3.Error, OSError):
+        return False

Optional: consider try/except/else/finally to move return True into else (TRY300).
As per static analysis hints.

---

323-338: Use SQL parameters; tame N+1 label lookups/noisy prints.

• Parameterize t.date to avoid string interpolation.
• Gate debug prints on verbose.
• Consider replacing per‑row label queries with a LEFT JOIN using group_concat to avoid N+1.

-            records = cursor.execute(f"""
+            records = cursor.execute("""
                 SELECT  t.transactionsTableID as id,
                         a.accountName AS account,
                         i.itemName AS desc,
                         t.amount AS value,
                         t.date AS date,
                         t.conversionRateNew AS rate
                 FROM TRANSACTIONSTABLE AS t
                 LEFT JOIN ACCOUNTSTABLE AS a ON t.accountID = a.accountsTableID
                 LEFT JOIN ITEMTABLE AS i ON t.itemID = i.itemTableID
                 WHERE   t.amount != 0
-                        AND t.date = '{date}'
+                        AND t.date = ?
                         AND t.transactionCurrency IN ({selected_currencies})
                 ORDER BY t.transactionsTableID ASC;
-            """).fetchall()
+            """, (date,)).fetchall()
@@
-                if labels:
-                    print('labels', labels)
+                if labels:
+                    if verbose:
+                        print('labels', labels)
                     desc1 += " - " + " - ".join(item[0] for item in labels)
@@
-                else:
-                    duplicated += 1
-                    print('duplicated', row)
+                else:
+                    duplicated += 1
+                    if verbose:
+                        print('duplicated', row)

Join idea (sketch): LEFT JOIN a subquery SELECT transactionIDLabels, group_concat(labelName, ' - ') AS labels FROM LABELSTABLE GROUP BY transactionIDLabels and coalesce into desc.

Also applies to: 344-349, 387-389

---

443-444: Avoid sys.exit inside library function.

Return a status code; let __main__ own process exit.

-        print('OK')
-        sys.exit(0)
+        print('OK')
+        return 0

---

42-63: Return strategy: avoid mixing valid values and error strings.

Consider raising ValueError (or returning None) on parse failure instead of returning an error string. Callers can handle errors explicitly.

---

162-229: Self-test hygiene: temp files and exceptions.

• Prefer tempfile.TemporaryDirectory() for test files; drop manual cleanup.
• Narrow catch from Exception to specific exceptions (BLE001).

Example sketch:

+import tempfile
@@
-def test_is_valid_sqlite_db():
+def test_is_valid_sqlite_db():
@@
-    valid_db_path = 'temp_test_valid.db'
-    invalid_file_path = 'temp_test_invalid.txt'
+    with tempfile.TemporaryDirectory() as tmp:
+        valid_db_path = os.path.join(tmp, 'valid.db')
+        invalid_file_path = os.path.join(tmp, 'invalid.txt')
@@
-    except Exception as e:
+    except (sqlite3.Error, OSError) as e:
         print(f"Setup Error: Failed to create valid DB file. Exiting. {e}")
         return
@@
-    except Exception as e:
+    except OSError as e:
         print(f"Setup Error: Failed to create invalid text file. Exiting. {e}")
         return
@@
-    finally:
-        if debug:
-            print("Starting cleanup...")
-        if os.path.exists(valid_db_path):
-            os.remove(valid_db_path)
-        if os.path.exists(invalid_file_path):
-            os.remove(invalid_file_path)
-        if debug:
-            print("Cleanup complete.")
+    # cleanup handled by TemporaryDirectory context manager

As per static analysis hints.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (1)

380-383: Use a set instead of list for better performance.

List membership checks (if index[i] not in same_account_transfer) are O(n), making this loop O(n²) with many transactions. Using a set would provide O(1) membership checks.

Apply this diff:

     # look for transfer to the same account
     index = sorted(rows)
-    same_account_transfer = []
+    same_account_transfer = set()
     for i in range(0, len(index)):
         if i > 0:
             account1, desc1, value1, date1, rate1, id1 = rows[index[i]]
             account2, desc2, value2, date2, rate2, id2 = rows[index[i - 1]]
             if account1 == account2 and date1 == date2 and abs(value1) == abs(value2):
                 if debug:
                     print('bad============================================')
                     print(i, index[i], rows[index[i]])
                     print(i-1, index[i - 1], rows[index[i - 1]])
                 if index[i] not in same_account_transfer:
-                    same_account_transfer.append(index[i])
+                    same_account_transfer.add(index[i])
                 if index[i - 1] not in same_account_transfer:
-                    same_account_transfer.append(index[i - 1])
+                    same_account_transfer.add(index[i - 1])

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (1)

496-499: Self-tests still run unconditionally; move under --self-test.

test_add_microseconds_and_format() and test_is_valid_sqlite_db() are executed on every run.

See consolidated main-guard diff above which gates tests strictly under --self-test.

playground/bluecoins.py (6)

330-344: Parameterize the date filter; avoid f-strings in SQL.

Safer and clearer; keeps currency list constant but binds date.

-            records = cursor.execute(f"""
+            records = cursor.execute("""
                 SELECT  t.transactionsTableID as id,
                         a.accountName AS account,
                         i.itemName AS desc,
                         t.amount AS value,
                         t.date AS date,
                         t.conversionRateNew AS rate
                 FROM TRANSACTIONSTABLE AS t
                 LEFT JOIN ACCOUNTSTABLE AS a ON t.accountID = a.accountsTableID
                 LEFT JOIN ITEMTABLE AS i ON t.itemID = i.itemTableID
                 WHERE   t.amount != 0
-                        AND t.date = '{date}'
+                        AND t.date = ?
                         AND t.transactionCurrency IN ({selected_currencies})
                         AND t.reminderTransaction IS NULL
                 ORDER BY t.transactionsTableID ASC;
-            """).fetchall()
+            """, (date,)).fetchall()

---

351-359: Gate label debug print behind verbosity.

Reduce noisy output during normal runs.

-                if labels:
-                    print('labels', labels)
+                if labels:
+                    if verbose:
+                        print('labels', labels)
                     desc1 += " - " + " - ".join(item[0] for item in labels)

---

421-428: In >2-items adjustment, base increment on each row’s timestamp, not outer group key.

Using date (group key) may ignore existing microseconds in date1.

-                    new_date = add_microseconds_and_format(date, y)
+                    new_date = add_microseconds_and_format(date1, y)

---

241-244: Open DB in read-only mode to prevent accidental writes.

Use SQLite URI with mode=ro.

-        conn = sqlite3.connect(db_file)
+        conn = sqlite3.connect(f"file:{db_file}?mode=ro", uri=True)

---

446-447: Write rows in deterministic order.

Stabilizes CSV output.

-            for _, rows in data.items():
+            for _, rows in sorted(data.items()):
                 csv_writer.writerows(rows.values())

---

349-350: Avoid runtime assert in production path.

Prefer explicit check/log to prevent unexpected termination.

-                assert id1 not in rows
+                if id1 in rows:
+                    if debug:
+                        print(f"duplicate id in date bucket: {id1}")
+                    continue

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (1)

461-488: Critical: Tests run unconditionally and verbose flag ignored.

Despite past review comments marking these as addressed, two critical issues remain:

1. Line 462: verbose = True is hardcoded, so the -v/--verbose CLI flag has no effect.
2. Lines 479-480: Tests execute on every run, even when just processing a database, causing unexpected delays.

The verbose variable should be set from args.verbose after parsing, and tests should only run inside the --self-test conditional.

Apply this diff:

 if __name__ == "__main__":
-    verbose = True
     parser = argparse.ArgumentParser(description="Process Bluecoins database and export data to CSV.")
     parser.add_argument("--self-test", action="store_true", help="Run module self-tests and exit")
     parser.add_argument("-v", "--verbose", action="store_true", help="Enable verbose output")
     parser.add_argument("db_file", nargs="?", help="Path to the Bluecoins database file (.fydb)")
     parser.add_argument("-y", "--yes", action="store_true", help="Proceed without confirmation prompt")
     args = parser.parse_args()
 
     # sys.argv is the list of command-line arguments.
     # sys.argv[0] is the script name itself, so we check if the list has only one item.
     if len(sys.argv) == 1:
         print("🚨 No arguments provided.")
         parser.print_help(sys.stderr) # Print help message to standard error stream (optional but common practice)
         sys.exit(1) # Exit the script with a non-zero status code (convention for failure)
 
     # Run the tests
+    verbose = args.verbose
     debug = args.self_test or args.verbose
-    test_add_microseconds_and_format()
-    test_is_valid_sqlite_db()
+    yes = args.yes
+    
     if args.self_test:
+        test_add_microseconds_and_format()
+        test_is_valid_sqlite_db()
         sys.exit(0)
+    
+    if not args.db_file:
+        print("error: db_file is required")
+        parser.print_help(sys.stderr)
+        sys.exit(1)
+        
     if not is_valid_sqlite_db(args.db_file):
         print(f"error: {args.db_file} is invalid sqlite3 database")
         sys.exit(1)
-    yes = args.yes
+    
     code = process_bluecoins_data(args.db_file)
     sys.exit(code if isinstance(code, int) else 0)

This also adds a check for missing db_file argument before validation.

playground/bluecoins.py (1)

115-159: Well-implemented DB validation with proper resource management.

The function correctly validates SQLite databases with existence checks, connection attempts, and schema verification. Resource cleanup is properly handled in the finally block.

The broad Exception catch on line 153 is acceptable as a fallback after the specific sqlite3.DatabaseError, though you could optionally narrow it to specific exceptions like PermissionError or OSError for more explicit error handling.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (2)

479-482: Tests run unconditionally on every execution.

The test functions execute on every run (lines 479-480), even when the user just wants to process a database. This was previously flagged and remains unfixed.

Move test execution inside the --self-test conditional:

     # Run the tests
     verbose = args.verbose
     debug = args.self_test or verbose
-    test_add_microseconds_and_format()
-    test_is_valid_sqlite_db()
     if args.self_test:
+        test_add_microseconds_and_format()
+        test_is_valid_sqlite_db()
         sys.exit(0)

---

483-485: Fix typo and remove redundant error message.

Two issues previously flagged that remain unfixed:

1. Typo: "invaild" should be "invalid" (line 484)
2. Redundant error: is_valid_sqlite_db prints its own error when verbose is enabled; this creates duplicate output
3. Missing None check: Since db_file is optional (nargs="?"), args.db_file could be None

Apply this diff:

-    if not is_valid_sqlite_db(args.db_file):
-        print(f"error: {args.db_file} is invalid sqlite3 database")
+    if not args.db_file:
+        print("error: database file path is required")
+        sys.exit(1)
+    if not is_valid_sqlite_db(args.db_file):
         sys.exit(1)

playground/bluecoins.py (1)

329-344: Consider using parameterized queries for better security practices.

While the current f-string interpolation is safe (data comes from the database and hardcoded values), using parameterized queries is a better security practice and makes the code more maintainable.

Consider refactoring to:

records = cursor.execute("""
    SELECT  t.transactionsTableID as id,
            a.accountName AS account,
            i.itemName AS desc,
            t.amount AS value,
            t.date AS date,
            t.conversionRateNew AS rate
    FROM TRANSACTIONSTABLE AS t
    LEFT JOIN ACCOUNTSTABLE AS a ON t.accountID = a.accountsTableID
    LEFT JOIN ITEMTABLE AS i ON t.itemID = i.itemTableID
    WHERE   t.amount != 0
            AND t.date = ?
            AND t.transactionCurrency IN (?, ?, ?)
            AND t.reminderTransaction IS NULL
    ORDER BY t.transactionsTableID ASC;
""", (date, 'SDG', 'SAR', 'USD')).fetchall()

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (1)

480-483: Fix: Tests run unconditionally on every execution.

Lines 480-481 execute test suites on every run, even when the user just wants to process a database. This slows down normal usage and produces unexpected output. Tests should only run when --self-test is specified.

Apply this diff:

     # Run the tests
     verbose = args.verbose
     debug = args.self_test or verbose
-    test_add_microseconds_and_format()
-    test_is_valid_sqlite_db()
     if args.self_test:
+        test_add_microseconds_and_format()
+        test_is_valid_sqlite_db()
         sys.exit(0)

playground/bluecoins.py (1)

116-157: Consider narrowing the exception handler scope.

The broad except Exception catch at line 151 could mask unexpected errors beyond database validation (e.g., programming errors). Consider catching only specific exceptions you expect (like OSError for permission errors) or at least logging unexpected exceptions before returning False.

Example refinement:

     except sqlite3.DatabaseError:
         # Catches specific errors indicating the file is not a valid SQLite format
         # (e.g., "file is not a database")
         return False
-    except Exception:
-        # Catch any other unexpected errors (like permission denied, etc.)
+    except (OSError, IOError) as e:
+        # Catch filesystem errors (permission denied, etc.)
+        if verbose:
+            print(f"error: Cannot access {db_path}: {e}")
         return False

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

playground/bluecoins.py (3)

484-485: Tests still run unconditionally on every execution.

These test functions execute before the --self-test check, meaning they run even when the user only wants to process a database file. This is unexpected behavior and slows down normal usage.

Move the test calls inside the conditional:

     # Run the tests
     verbose = args.verbose
     debug = args.self_test or verbose
-    test_add_microseconds_and_format()
-    test_is_valid_sqlite_db()
     if args.self_test:
+        test_add_microseconds_and_format()
+        test_is_valid_sqlite_db()
         sys.exit(0)

---

488-490: Remove redundant error message.

The is_valid_sqlite_db function already prints an error message when verbose mode is enabled (line 129). This additional print creates duplicate output and violates separation of concerns.

Apply this diff:

     if not is_valid_sqlite_db(args.db_file):
-        print(f"error: {args.db_file} is invalid sqlite3 database")
         sys.exit(1)

---

476-479: Fix argument validation logic.

The condition len(sys.argv) <= 1 is incorrect. It should be == 1 to only show help when no arguments are provided. The current check would also show help if exactly one argument is provided, which is incorrect since valid usage includes single flags like --self-test.

Apply this diff:

     # sys.argv is the list of command-line arguments.
     # sys.argv[0] is the script name itself, so we check if the list has only one item.
-    if len(sys.argv) <= 1:
+    if len(sys.argv) == 1:
         print("🚨 No arguments provided.")
         parser.print_help(sys.stderr) # Print help message to standard error stream (optional but common practice)
         sys.exit(1) # Exit the script with a non-zero status code (convention for failure)

playground/bluecoins.py (2)

330-345: Use parameterized queries to avoid SQL injection risk.

The query embeds date and selected_currencies using f-strings. While these currently come from trusted sources, this pattern is fragile and creates security risk if the data flow changes.

Refactor to use parameterized queries:

             records = cursor.execute(f"""
                 SELECT  t.transactionsTableID as id,
                         a.accountName AS account,
                         i.itemName AS desc,
                         t.amount AS value,
                         t.date AS date,
                         t.conversionRateNew AS rate
                 FROM TRANSACTIONSTABLE AS t
                 LEFT JOIN ACCOUNTSTABLE AS a ON t.accountID = a.accountsTableID
                 LEFT JOIN ITEMTABLE AS i ON t.itemID = i.itemTableID
                 WHERE   t.amount != 0
-                        AND t.date = '{date}'
-                        AND t.transactionCurrency IN ({selected_currencies})
+                        AND t.date = ?
+                        AND t.transactionCurrency IN ('SDG', 'SAR', 'USD')
                         AND t.reminderTransaction IS NULL
                 ORDER BY t.transactionsTableID ASC;
-            """).fetchall()
+            """, (date,)).fetchall()

---

381-388: Clarify duplicate counting logic.

The duplicate counting logic increments duplicated when a transaction is already in same_account_transfer, but this occurs during the detection phase, not the removal phase. This may count the same transaction multiple times across different comparisons, leading to an inflated count.

Consider refactoring for clarity:

                     if account1 == account2 and date1 == date2 and abs(value1) == abs(value2):
                         if debug:
                             print('bad============================================')
                             print(i, index[i], rows[index[i]])
                             print(i-1, index[i - 1], rows[index[i - 1]])
-                        if index[i] in same_account_transfer:
-                            duplicated += 1
-                        else:
-                            same_account_transfer.append(index[i])
-                        if index[i - 1] in same_account_transfer:
-                            duplicated += 1
-                        else:
-                            same_account_transfer.append(index[i - 1])
+                        if index[i] not in same_account_transfer:
+                            same_account_transfer.append(index[i])
+                        if index[i - 1] not in same_account_transfer:
+                            same_account_transfer.append(index[i - 1])

Then count duplicates only during removal (after line 389):

             # remove "same account transfer" records
             if same_account_transfer:
+                duplicated += len(same_account_transfer)
                 if debug:
                     print('same_account_transfer', same_account_transfer)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro