-
Notifications
You must be signed in to change notification settings - Fork 25
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
Fix issue with minimum-perc skip message #157
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.
Generally it's fine. I am approving this so if you want to discard my suggestion, go ahead and merge.
I don't like how the return values are inconsistent. Instead of (pseudocode in python-like language for speed):
def main():
should_skip, msg = determine_if_should_skip()
if should_skip:
if msg:
print(msg)
else:
print("Some default message")
def determine_if_should_skip():
if some_condition:
return True, ""
elif some_other_condition:
return True, "A very specific message"
else:
return False, ""
I think I would prefer
def main():
should_skip, msg = determine_if_should_skip()
if should_skip:
print(msg)
def determine_if_should_skip():
if some_condition:
return True, "A default message"
elif some_other_condition:
return True, "A very specific message"
else:
return False, None
Yes this would mean that we would have to use the default message a few times inside the body of shouldSkipDownload
but if this bothers you, you can define both messages as constant variables at the top of the function and use the variables when returning.
67a8023
to
ad9fbbc
Compare
Before the fix, we were returning a message that a local file is newer than the remote one, which wasn't true. This fix updates the `shouldSkipDownload` to return an additional value with a message in case it's different than the default one. Also fixed a typo in readme regarding the `minimum-perc` flag.
ad9fbbc
to
84e2e99
Compare
@kbairak Nahh, good feedback. I didn't like something either. 🙏 |
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.
That works (again approving in case you want to discard), but...
Lets try:
func shouldSkipDownload(...) (bool, string, error) {
const minimumThresholdMessage = "Minimum translation completion threshold not satisfied, skipping"
const newerThanRemoteMessage = "Local file is newer than remote, skipping"
if something {
return true, minimumThresholdMessage, nil
} else if somethingElse {
return true, newerThanRemoteMessage, nil
} else {
return false, "", nil
}
}
should bring better log messages see transifex/cli#157
Before the fix, we were returning a message that a local file is newer than the remote one, which wasn't true.
This fix updates the
shouldSkipDownload
to return an additional value with a message in case it's different than the default one.Also fixed a typo in readme regarding the
minimum-perc
flag.Addresses #155