Skip to content

It's easier to manage vars through dotenv #5

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.

Sign up for GitHub

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

Open
Dimlock wants to merge 1 commit into
base: master
Choose a base branch
from
Open

It's easier to manage vars through dotenv #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ollamautil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import ast
import argparse
import hashlib

from dotenv import load_dotenv
from prettytable import PrettyTable
from tqdm import tqdm as tqdm
from typing import List, Tuple
Expand Down Expand Up @@ -390,6 +392,8 @@ def convert_bytes(size):
if not overwrite:
# Remove the existing files from the list of files to copy
files_to_copy = [f for f in files_to_copy if f not in existing_files]
else:
overwrite = False
Comment on lines +395 to +396
Copy link

@gemini-code-assist gemini-code-assist bot Dec 1, 2025

Choose a reason for hiding this comment

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

medium

This else block correctly fixes a potential UnboundLocalError for the overwrite variable. A more common and arguably cleaner pattern is to initialize variables with a default value. Consider initializing overwrite = False before the if existing_files: block on line 388 and removing this else block. This makes the default value explicit and improves readability.


if not files_to_copy:
print("No files to copy. Exiting copy operation.")
Expand Down Expand Up @@ -746,11 +750,14 @@ def main() -> None:
"external caches, respectively.]]")
# define the default source directories (no training delimiter)
# points to path of internal "modules" directory
load_dotenv()
global ollama_int_dir
ollama_int_dir = os.getenv("OLLAMAUTIL_INTERNAL_DIR")
# points to path of external "modules" directory
global ollama_ext_dir
ollama_ext_dir = os.getenv("OLLAMAUTIL_EXTERNAL_DIR")
if ollama_int_dir is None or ollama_ext_dir is None:
raise Exception("Ollamautil internal and external directories not set.")
Comment on lines +759 to +760
Copy link

@gemini-code-assist gemini-code-assist bot Dec 1, 2025

Choose a reason for hiding this comment

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

high

This check is a good addition, but it can be improved. The check is None will not catch empty strings which are also likely invalid for directory paths. Also, there is a redundant check on lines 773-779. Consider using a single, more robust check and a more specific exception type like ValueError.

Furthermore, the same logic for loading and checking environment variables is duplicated in src/ollamautil/cli.py. This logic could be extracted into a shared utility function to follow the DRY (Don't Repeat Yourself) principle.

I'd suggest replacing this check with the one below, and removing the redundant one at lines 773-779.

Suggested change
if ollama_int_dir is None or ollama_ext_dir is None:
raise Exception("Ollamautil internal and external directories not set.")
if not ollama_int_dir or not ollama_ext_dir:
raise ValueError("Ollamautil internal and external directories not set. Please set OLLAMAUTIL_INTERNAL_DIR and OLLAMAUTIL_EXTERNAL_DIR in your environment or .env file.")

global valid_caches
valid_caches = [
('internal', ollama_int_dir),
Expand Down
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
python-dotenv
argparse==1.4.0
ollama==0.1.8
prettytable==3.10.0
Expand Down
8 changes: 6 additions & 2 deletions src/ollamautil/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from .ollamautil import build_ext_int_comb_filelist, display_models_table, migrate_cache_user, toggle_int_ext_cache, remove_from_cache, pull_models, push_models
import os
import ast
from dotenv import load_dotenv

def main_menu():
print("\n\033[1mMain Menu\033[0m")
Expand Down Expand Up @@ -39,12 +40,15 @@ def process_choice(choice: str, combined, models_table: PrettyTable|None = None)
print("Invalid choice, please try again.")

def main() -> None:
'''
"""
Display main menu and basic high-level handling.
'''
"""
# Set up environment variables
load_dotenv()
ollama_int_dir = os.getenv("OLLAMAUTIL_INTERNAL_DIR")
ollama_ext_dir = os.getenv("OLLAMAUTIL_EXTERNAL_DIR")
if ollama_int_dir is None or ollama_ext_dir is None:
raise Exception("Ollamautil internal and external directories not set.")
Comment on lines +50 to +51
Copy link

@gemini-code-assist gemini-code-assist bot Dec 1, 2025

Choose a reason for hiding this comment

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

high

This new check is redundant with the existing check on lines 58-62. The existing check is more robust as it also handles empty strings and provides a more informative error message before exiting with SystemExit, which is suitable for a CLI tool. It's best to consolidate these into a single check.

This logic is also duplicated in ollamautil.py. Consider creating a shared function for loading and validating environment variables to keep your code DRY.

I recommend removing this new if block and relying on the more comprehensive check on lines 58-62.

try:
ollama_file_ignore = ast.literal_eval(os.getenv("OLLAMAUTIL_FILE_IGNORE", "['.DS_Store']"))
except:
Expand Down