It's easier to manage vars through dotenv#5
Conversation
Summary of ChangesHello @Dimlock, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the application's configuration management by introducing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces python-dotenv for managing environment variables, which is a great step towards better configuration management. The changes correctly add the dependency and load variables from a .env file. My review focuses on improving code quality by addressing some code duplication and redundant logic that was introduced. Specifically, the environment variable loading and validation logic is duplicated in ollamautil.py and src/ollamautil/cli.py. I've provided suggestions to consolidate these checks and make them more robust. I also included a minor suggestion to improve code style for variable initialization.
| if ollama_int_dir is None or ollama_ext_dir is None: | ||
| raise Exception("Ollamautil internal and external directories not set.") |
There was a problem hiding this comment.
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.
| 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.") |
| if ollama_int_dir is None or ollama_ext_dir is None: | ||
| raise Exception("Ollamautil internal and external directories not set.") |
There was a problem hiding this comment.
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.
| else: | ||
| overwrite = False |
There was a problem hiding this comment.
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.
No description provided.