Issues (1-7) Resolved #16
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @keshxvdayal, your pull request is larger than the review limit of 500000 diff characters
uniqueg
left a comment
uniqueg
left a comment
There was a problem hiding this comment.
At a cursory look, it seems like the PR introduces way more changes than the ones outlined in the description. Why is that?
Obviously, this PR is impossible to review thoroughly, which highlights the importance of keeping PRs short and semantically focused on a single issue.
I have only looked at the general structure of the repo and addressed a few minor issues that hit my eye. There is a lot of work to do on the code base quality, and we can't do it all in this PR - but we have to start somewhere. Please clean out all the unnecessary files, address the few other comments I have made, and ask for a re-review.
.DS_Store
Outdated
There was a problem hiding this comment.
Please add a .gitignore file and add patterns for all types that shouldn't be under version control. Best to use a service like https://www.toptal.com/developers/gitignore/, then manually update, where necessary.
Please remove all .DS_Store, Python cache files etc. manually from version control and update the PR. We don't want to be distracted with such files.
There was a problem hiding this comment.
You're right, and I apologize for the noise. The PR includes restructuring the entire backend architecture (moving from monolithic app.py to modular routes/services/middleware structure), which wasn't fully detailed in the description. I should have broken this into multiple PRs:
(1) architectural restructuring,
(2) new features.
Additionally, I accidentally committed Python cache files and .DS store - I'll clean these immediately and update the .gitignore
There was a problem hiding this comment.
Thanks for adding the .gitignore, but the wrongly added files are still in the diff. Please do as Alex says here:
Please remove all .DS_Store, Python cache files etc. manually from version control and update the PR. We don't want to be distracted with such files.
There was a problem hiding this comment.
@ninsch3000 I have deleted the .DS_Store files. now the PR is ready for merge
There was a problem hiding this comment.
Why would you delete the README.md file?
There was a problem hiding this comment.
That was unintentional, I must have deleted it during the restructuring when moving files around. The original README had excellent documentation (411 lines covering features, architecture, API endpoints). I'll restore it from the main branch and update it to reflect the new modular architecture. Documentation should always be updated alongside code changes, not deleted.
There was a problem hiding this comment.
This should probably not be under version control
backend/app_old.py
Outdated
There was a problem hiding this comment.
Why keep multiple versions of modules around? That's what we use Git for. Please only keep a single version of each file.
There was a problem hiding this comment.
alright i will do that, i kept old app.py as a 'safety net' during the refactoring 😅
There was a problem hiding this comment.
What's with all the empty files?
|
@uniqueg i have pushed the commit having all the required changes as done. |
keshxvdayal
force-pushed
the
keshav-dev
branch
from
3a3aabd to
d8ef95c
Compare
Issues Resolved