Bug list size overload #37
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Akopti8
requested review from
ShaneMPutnam and
slawler
and removed request for
slawler
ShaneMPutnam
requested changes
Jun 12, 2024
There was a problem hiding this comment.
Looks of nice updates in here! See inline comments and some general comments here:
- There are a few places where you use errors.New instead of fmt.Errorf. See line 145 of move.go for example. Try to be consistent
- Sometimes your errors/return messages start with lower case letter and other times they start with a capital letter.
- Most of the time you use a colon followed by the error message, though sometimes you use a comma.
Note that I skimmed the test scripts, but did not test them since I am not sure how to do that. Also I do not totally understand the logic behind how the test names are numbered, at times they seem to be inconsistent, but I could just not get it.
ShaneMPutnam
approved these changes
Jun 12, 2024
There was a problem hiding this comment.
Thanks for those updates! I made a few small changes: 0c976c9 and one inline comment to consider as a part of your next PR. I also added or modified the following two issues for future updates:
| @@ -27,6 +27,31 @@ type ListResult struct { | |||
| ModifiedBy string `json:"modified_by"` | |||
| } | |||
|
|
|||
| // CheckAndAdjustPrefix checks if the prefix is an object and adjusts the prefix accordingly. | |||
| // Returns the adjusted prefix, an error message (if any), and the HTTP status code. | |||
| func CheckAndAdjustPrefix(s3Ctrl *S3Controller, bucket, prefix string) (string, string, int) { | |||
There was a problem hiding this comment.
I think it would be more standard to return an error, instead of a errorMessage? Like (string, int, error)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GetListWithCallBackfor safer retrieval of object lists, reducing memory usage compared toGetList. Resolves OptimizeGetListFunction for Large Dataset Handling #36