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

Refactor to core and cli #8

Merged
merged 5 commits into from
Oct 31, 2023
Merged

Refactor to core and cli #8

merged 5 commits into from
Oct 31, 2023

Conversation

ChaoticRoman
Copy link
Owner

@ChaoticRoman ChaoticRoman commented Oct 30, 2023

Split into core and cli for better logic reuse in future GUI app.

@github-actions
Copy link

Prompt too long for OpenAI: 4326 characters, sending only first 4000 characters
The code changes seem to be well structured and organized. Here are some suggestions and observations:

  1. The code is refactored into a more object-oriented style, which is good for code reusability and modularity. The GptCore class can now be used in other parts of the codebase if needed.

  2. The cli.py file has been significantly simplified which improves readability.

  3. The addition of __pycache__ and *.pyc to .gitignore is a good practice to avoid committing unnecessary files to the repository.

  4. The deletion of the .github/workflows/code-review.yml file means that the GitHub Actions workflow for code review will no longer be in effect. Make sure this is the intended behavior.

  5. The openai.api_key is read directly from the file in core.py. It would be better to use environment variables for storing such sensitive data.

  6. The GptCore class uses dependency injection for input and output which is a good practice for testing and flexibility.

  7. The Info dataclass seems to be incomplete as it's missing fields and also it's not used anywhere in the code.

  8. There's no error handling in the main method of the GptCore class. It would be good to add some error handling there, especially around the API call.

  9. The code lacks comments, which makes it harder to understand the purpose of certain sections of the code. Adding comments would improve code readability and maintainability.

  10. There's no newline at the end of the cli.py file. It's a common convention to end a file with a newline.

Overall, the code is well-structured and readable, but could benefit from some improvements.

@github-actions
Copy link

This is a solid code change overall and I like the separation of concerns by creating a new core.py file and GptCore class. This makes the code more modular and easier to maintain. Here are some points of feedback:

  1. Good job adding __pycache__ and *.pyc to .gitignore. This prevents compiled Python files from being committed to the repository.

  2. In cli.py, the function cli_input is a good abstraction for getting user input.

  3. In cli.py, the function cli_output is a good abstraction for outputting to the console.

  4. In core.py, the GptCore class is well-structured. It encapsulates the logic of interacting with the GPT-4 model and handles the pricing calculations.

  5. The use of the walrus operator (:=) in the while loop in the main method of GptCore is a modern Python feature that can make code more concise. However, it might be less readable to developers who are not familiar with it.

  6. The Info data class is a good use of Python's built-in dataclasses for simple data structures. It provides a clear, concise way to define a data type with a small amount of boilerplate code.

  7. One thing to consider is error handling. What happens if openai.ChatCompletion.create fails? It might be good to add some error handling code here.

  8. The os.chdir(os.path.dirname(__file__)) code at the top of core.py changes the working directory of the script to the directory of the script file. This could potentially cause issues if other parts of your program are expecting the working directory to be somewhere else. If the goal is to ensure that .api_key is read from the same directory as the script file, consider using os.path.join(os.path.dirname(__file__), '.api_key') instead of changing the working directory.

  9. It's not clear what temperature=0 does in the call to openai.ChatCompletion.create. It would be helpful to add a comment explaining this.

  10. It's also not clear what happens if the user types 'q', 'x', 'quit', or 'exit'. In the original code, these were handled explicitly, but in the new code, it seems like they would be treated as regular prompts. If you want to keep the same behavior, you might need to check for these inputs in the main method of GptCore.

@github-actions
Copy link

The code changes are generally good, as they improve the separation of concerns and make the code more modular. Here are some specific points:

  1. The code has been refactored into separate functions and classes, which improves readability and maintainability. The new GptCore class and the cli_input and cli_output functions in cli.py make the code more modular and easier to test.

  2. The .gitignore file has been updated to ignore Python cache files, which is a good practice.

  3. The api_key is now being loaded in core.py instead of cli.py, which makes sense since core.py is where the OpenAI API is being used.

  4. The while loop has been moved into the main method of the GptCore class, which is a good practice as it encapsulates the main logic of the program into a single method.

  5. The Info class has been added to encapsulate the information about the prompt tokens, completion tokens, and price. This is a good use of a data class and makes the code cleaner and easier to understand.

  6. The TEMPERATURE constant has been added but is set to 0.0. This might not be what you want as it would make the model deterministic and it won't generate any creative outputs.

  7. The core.GptCore(cli_input, cli_output).main() line at the end of cli.py is a bit hard to read, you might consider breaking it up into multiple lines for clarity.

  8. It would be good to add some comments to your code to explain what each part does, especially for the GptCore class and its methods. This will make the code easier to understand for other developers.

  9. The openai import is moved to core.py which is a good practice.

  10. The error handling is missing. What if the openai.ChatCompletion.create method throws an error? It would be good to wrap it in a try/except block and handle any potential errors.

@github-actions
Copy link

Prompt too long for OpenAI: 4228 characters, sending only first 4000 characters
The code changes seem to be well-structured and follow good practices. Here are some points to consider:

  1. The code has been refactored to move the core logic into a separate core.py file which is a good practice. It separates concerns and makes the code more modular.

  2. The cli.py file has been simplified to only handle user input and output, which is a good practice. However, it might be more readable to rename the functions cli_input and cli_output to something more descriptive, like get_user_input and display_output.

  3. The core.py file handles the main logic of the application. The use of a GptCore class is a good practice as it encapsulates related functionality. However, it might be more readable to rename the input and output arguments to something more descriptive, like user_input_function and output_function.

  4. In the .github/workflows/code-review.yml file, the model and max-length values have been changed. Make sure these new values are appropriate for your use case.

  5. In the .gitignore file, __pycache__ and *.pyc have been added, which is a good practice to prevent Python bytecode files from being committed to the repository.

  6. The Info class in core.py is defined but not used. If it's meant to be used for the output function in cli.py, it's not currently being used that way.

  7. The code ends abruptly after the Info class. If there's more code that got cut off, please provide the full code.

Overall, the code is clean, but there are some minor improvements that could be made for readability. Also, ensure that all code and changes are complete and nothing is missing.

@github-actions
Copy link

Prompt too long for OpenAI: 4167 characters, sending only first 4000 characters
The code changes look good overall, but here are some suggestions and potential improvements:

  1. In the cli.py file, the cli_input() function can be improved by adding a prompt to the input() function. This will help the user understand that the program is waiting for their input. For example:
def cli_input():
    user_input = input("Enter your command: ")
    if user_input in ('q', 'x', 'quit', 'exit'):
        return None
    return user_input
  1. In the core.py file, the GptCore class has a method main() which can be renamed to a more descriptive name like run_chat() or start_chat(). This will make the code more readable and understandable.

  2. In the core.py file, it's good that you have separated the API key loading into a separate variable api_key_path, but it would be better to put this into a separate function. This way, if the way you load the API key changes in the future, you only need to change it in one place.

  3. The core.py file is missing an import for dataclasses. Be sure to add from dataclasses import dataclass at the top of your file.

  4. The Info class in core.py is missing the end of the __repr__ method. This is a critical issue because it will cause a syntax error. Make sure to complete this method.

  5. It's a good practice to add comments to the code to explain what each part is doing, especially for complex sections. This will make the code easier to understand for other developers.

  6. Make sure to handle potential exceptions. For example, an error might occur when reading the API key or when making a request to the OpenAI API. You should add appropriate error handling for these cases to improve the robustness of your code.

  7. Remember to follow the PEP8 style guide for Python code. This includes things like having two blank lines between function and class definitions, and having spaces around operators.

@ChaoticRoman ChaoticRoman self-assigned this Oct 31, 2023
@ChaoticRoman ChaoticRoman merged commit 09fac85 into main Oct 31, 2023
1 check passed
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.

1 participant