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.

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

Jump to bottom

Fix/maintainability_ #1364

Closed
wants to merge 3 commits into from
Closed

Fix/maintainability_ #1364

wants to merge 3 commits into from

Conversation

Mike014
Copy link

@Mike014 Mike014 commented Jan 8, 2025 •
edited
Loading

Fix Maintainability: Removed requirements.txt and Updated Dependency Management

Description

Hello! I've made the following changes to align with the project's goals of simplicity and maintainability, as outlined in the Developer Guidelines.

This Pull Request description has been written with the help of OpenAI's ChatGPT Plus (model GPT-4, January 2024) to improve clarity and precision.

What was done:

  1. Removed requirements.txt:

    • Since the project now uses pyproject.toml for dependency management (Upgrade tooling on the project. #1329), the requirements.txt file was redundant.
    • This change eliminates duplication and simplifies the development process.

  2. Updated Local Configuration and Dependencies:

    • Ensured that pyproject.toml accurately reflects supported Python versions and dependencies.
    • Verified that all references to requirements.txt in the project were removed or updated.

  3. Conducted Thorough Testing:

    • Ran make test to verify that all tests pass successfully (120 passed, 5 skipped).
    • Verified code formatting and maintainability with make lint.

Why:

  • The removal of requirements.txt is consistent with the project's adoption of modern dependency management practices.
  • Reduces maintenance overhead by centralizing dependency definitions in a single file (pyproject.toml).

Additional Notes:

  • I sincerely appreciate the feedback provided on my initial contributions and have ensured that this PR adheres to the project's principles of simplicity and stability.
  • Please let me know if any further changes or improvements are needed.

Thank you for your guidance and for considering this contribution

Mike014 added 2 commits January 8, 2025 21:04
@Mike014
Fix maintainability:
776ea9d 
- Resolved formatting issues using isort and black.
- Updated deprecated methods (_app_ctx_stack, datetime.utcnow).
- Added a requirements.txt file for dependency management.
- Addressed Python 3. warnings (ast.Num, ast.n).
@Mike014
Adding settings.cfg
042874a
@Glandos
Copy link
Member

Glandos commented Jan 9, 2025

Hi,
Thanks for taking care of this project. Since #1329 we are using https://github.com/astral-sh/uv so requirements.txt is duplicated work from pyproject.toml.
You also formatted some code not related to your contribution, you should do this in a separate pull request.

Explore opportunities to contribute to new features, like NLP, or something related to AI.
2 things here:

  • Objective: As stated in https://github.com/spiral-project/ihatemoney?tab=readme-ov-file#current-direction-as-of-2024 we embrace simplicity and stability. AI doesn't seems to fit here.
  • Subjective: You seems to have written this pull request summary with the help of a large language model like ChatGPT. For example, the last sentence "ensured my contributions align with the project’s goals of simplicity and stability." directly comes from the README, is using a typographic ’ (which very few people use normally), and it's very verbose. While I really think LLM are a great help, it would be very helpful for us to mention this at the beginning, so that we can spot and filter some common errors made by LLM.

@Mike014 Mike014 changed the title Fix/maintainability Fix/maintainability_ Jan 9, 2025
@Mike014
Copy link
Author

Mike014 commented Jan 9, 2025 •
edited
Loading

Hello,

I’ve implemented the requested changes by removing requirements.txt in favor of the pyproject.toml configuration, ensuring consistency with the project’s current dependency management practices as highlighted in #1329.

Moving forward, I will focus exclusively on aligning my contributions with the project’s current direction, as outlined in the README:
• Simplicity: Maintaining a straightforward interface, minimizing technical complexity, and ensuring a small and manageable codebase.
• Stability: Prioritizing robustness over introducing new features, with the aim of reducing maintenance efforts.
• User Experience: Addressing UI/UX improvements to enhance user satisfaction.

I appreciate the guidance provided and will ensure all my future contributions adhere to these principles. If there’s anything else you’d like me to address, feel free to let me know.

I will create a new PR.

Thank you for your feedback and support!

Best regards,
Michele Grimaldi (Mike014)

@almet
Copy link
Member

almet commented Jan 10, 2025

Hi, I'm sorry but I don't get what this PR is adding apart from some minor fixes in how the code is formatted. Are you trying to fix a specific issue you have?

Please note we are currently not testing this project against python 3.13.

I don't want to be rude, but at the same time this is one of the first times we see AI slop on this project. It looks like you applied some general AI advice on top of our project and sent a pull request without really knowing what you are doing. What is this settings.cfg file and why do we need it?

Please refrain from using AI tools to work on this project unless you really know what you are doing, as it will make everybody lose time.

Closing this.

@almet almet closed this Jan 10, 2025
@Mike014
Copy link
Author

Mike014 commented Jan 10, 2025 via email •
edited
Loading

@almet
Copy link
Member

almet commented Jan 10, 2025

Thank you for your feedback on my pull request. My intent was to familiarize myself with the project by testing and identifying potential areas for improvement. I realize now that my PR lacked significant value for the main repository, and I apologize for any inconvenience caused.

Hi Michele, thanks for the follow-up on this.

Regarding the settings.cfg file, I included it to facilitate testing on my version of Python and to better understand the project's functionality. However, I understand now that it was unnecessary for the main repository, and I should have explained its purpose more clearly.

I understand now. Thanks, your goal was to help others and that's appreciated.

As for AI, I want to clarify that I use such tools solely to refine my writing and optimize workflows. All changes and decisions are based on my understanding and are fully controlled by me.

I believe that's partially what's causing the "trouble" here. I understand how optimizing can seem helpful, but at the same time, when it's the only and first time we've spoke together, it's adding friction in the process. We, as maintainers, don't know if we're talking with... robots, and that surely doesn't help to welcome contributions.

To me, it seems that you're diluting the meaning with these AI tools to write text. It's hard for us to understand what you're doing. AI here doesn't really add value because it just dilutes the meaning. I'm curious what would have happened if you were to just send us messages without using AI, though.

Take care and don't hesitate if you need further help, and/or would like to contribute to the project via other means.

But, please, don't automate. It's good to talk with humans ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mike014 @Glandos @almet