Con 1870 logging message for no file downloaded#94
Conversation
Summary of ChangesHello @az-smartling, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the file download process by improving how translation progress is calculated and managed. It introduces more robust parsing for progress parameters, updates the completion percentage calculation for files, and adds clear logging for files that are skipped because they do not meet the specified progress threshold. This enhances the transparency and reliability of the file pulling service. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces logging for skipped file downloads and modifies the progress calculation logic. My review focuses on improving the robustness of the new logic. I've identified a critical issue that could lead to a panic due to a potential division-by-zero error in the progress calculation. Additionally, I've found a bug in how the progress percentage string is parsed and a typo in a new variable name. Applying these suggestions will make the code more resilient and maintainable.
| if strings.HasSuffix(params.Progress, "%") { | ||
| params.Progress = strings.TrimSpace(strings.TrimSuffix(params.Progress, "%")) | ||
| } | ||
| params.Progress = strings.TrimSuffix(params.Progress, "%") |
There was a problem hiding this comment.
The logic for cleaning the params.Progress string is complex and has a bug. For example, if params.Progress is "50% " (with a trailing space), strings.HasSuffix will be false, and the trailing space will not be removed. This will cause strconv.ParseInt to fail later. This logic can be simplified and made more robust.
params.Progress = strings.TrimSpace(params.Progress)
params.Progress = strings.TrimSuffix(params.Progress, "%")| params.Progress = "0" | ||
| } | ||
|
|
||
| persentByExcludedFile := make(map[string]string) |
There was a problem hiding this comment.
There's a typo in the variable name persentByExcludedFile. It should be percentByExcludedFile. This typo is present in other parts of the code where this variable is used (lines 173 and 199).
percentByExcludedFile := make(map[string]string)| if complete < percents { | ||
| continue | ||
| if total := locale.CompletedStringCount + locale.AuthorizedStringCount; total > 0 { | ||
| complete = int64( |
There was a problem hiding this comment.
Pls add unit tests. The formula is tricky, especially rounding. Proposed tests cover this and other cases.
|
|
||
| if percents > 0 { | ||
| if complete < percents { | ||
| percentByExcludedFile[path] = strconv.Itoa(int(complete)) |
There was a problem hiding this comment.
Why not simply call here fmt.Printf("skipped.... I don't understand how this construction works. But it is totally fine to mix downloaded and skipped messages.
| params.Progress = strings.TrimSpace(params.Progress) | ||
| params.Progress = strings.TrimSpace(strings.TrimSuffix(params.Progress, "%")) |
There was a problem hiding this comment.
Just an idea. Instead of modifying the input object, I can propose to define a new local var progressThreshold. It has a better name and int type. So no guess if it string or something else.
…oaded # Conflicts: # go.sum
Added logging message for no file downloaded
Changed calculation of progress
Smartling/api-sdk-go#23