-
Notifications
You must be signed in to change notification settings - Fork 6
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
Optimization #28
base: main
Are you sure you want to change the base?
Optimization #28
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.
I answered and resolved the comments on #26 and will get to creating the 2 requested unit tests by professor and create an extra third unit test, then I will then start working on the EDIS poster if all checks pass. |
"Valid File Not Redownloaded"
|
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.
Overall, I think the sentiment is the same here: some of these are interesting changes, but we really want to see a PR that is only the "don't re-download if the file already exists in the filesystem and validates properly".
Please move the other improvements / fixes to other PRs where they can be discussed separately.
hey thank you for listening to, and reading my comments :) |
thank you for being patient with this PR, i fixed most of the CI test errors :) |
may i get the CI test to run please, thank you so much and apologies for bugging you i just want to make sure i get my part of the code completed before EDIS |
Hey! after our meeting today i decided to remove and revert some things that we discussed in our meeting today @jsquyres After reverting my code that i did to fix the CI test on lock_test.go and delete_test.go leaving just resource.go and dowload_test.go. I now have the same error that i had before i had made the changes to my code to fix the CI test so that it would pass, now i have a "go test ./cmd -v" error in: FAIL: TestRunDownloadMultipleErrors These were the tests that i had fixed by adding lock_test.go and delete_test.go. I am 100% positive that the CI test will not pass because of this change by deleting the other go files. I am not sure how to proceed further to closing this PR i would like some guidance. @rabadin @jsquyres Thank You!! |
thank you so much for letting me know, i accidentally figured it out while checking the goland ID, i removed it and hopefully the CI test passes now :) best regards |
if this CI test passes i will squash my 30 commits down to one commit hopefully without error |
if a file is already i the file system and it validates, we do not need to download it again.
Hello Raphael, Me and Dr.Squyres during our meeting today we figured out the problem with why the commits were not squashing after a bit of troubleshooting, Thanks to Dr.Squyres and his help I managed to get the 35 commits squashed down to 22 commits then 1 commit, all the while preserving the changes in the process. We both have requested a review for this PR on your end to approve this PR for the merge. Best Wishes and Happy Thanksgiving! |
No description provided.