-
Notifications
You must be signed in to change notification settings - Fork 63
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
updated Dry Run feature #128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityakalburgi I would suggest you follow the PR guidelines. Fill the template I have given here. It is needed so as to facilitate point assignment and any future review.
Also, please maintain a clean commit history by squashing all the commits. You can do so by follwowing the steps below:
git fetch origin
git rebase -i HEAD~5
squash <commit message>
git push --force
Wait for review from Karthik afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityakalburgi
please follow the PR template
only then any changes would be reviewed
Waiting for a response @adityakalburgi. Failure in responding within 24 hours will lead to me closing your PR. |
Hi i am soo updating the PR thank you.. |
@adityakalburgi |
@adityakalburgi update? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jull sync your fork and pull changes once to ensure you are up-to-date with everything and then we will be happy to merge your PR!
Ok sure will do it soon... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityakalburgi
please make changes as requested!
hi @techy4shri i have sync my fork please have a look thank you! |
@adityakalburgi will do soon. Wait for review from Karthik. |
@Kota-Karthik review ra pls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityakalburgi
Your pr still removes most of the code
like logging and other interactive cli code
please fix it!
from twinTrim.dataStructures.fileFilter import FileFilter | ||
|
||
# Setting up logging configuration | ||
logging.basicConfig ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityakalburgi
check this
your changes removes the logging part and interactive cli part totally
This is not cause you removed them , but because you havent pulled chaanges
updated the logging error
hi @Kota-Karthik i have updated my PR! |
I will say to close this PR, delete your fork of TwinTrim, Then refork the repo, do your changes on the new fork and then submit a new PR This PR now is looking dirty with so many redundant merges. |
Squash the commits and make the commit history clean please or raise a new PR. |
@adityakalburgi |
I will create a new PR so you can close this one |
Hi @techy4shri, @Kota-Karthik
Thank you for your feedback! I apologize for not following the PR template initially. I’m currently updating the PR with the required information.
Description
Updated the README.md to clarify project setup instructions.
Added a dry run feature in flags.py, which will display the files that would be deleted without actually removing them. This allows users to review the potential deletions before making any changes.
Type of change
Please delete options that are not relevant.
Checklist
Screenshots (if applicable)