Fix issue with minimum-perc skip message#157
Merged
Conversation
kbairak
previously approved these changes
Feb 15, 2023
kbairak
left a comment
There was a problem hiding this comment.
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, NoneYes 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
Contributor
Author
|
@kbairak Nahh, good feedback. I didn't like something either. 🙏 |
kbairak
approved these changes
Feb 15, 2023
There was a problem hiding this comment.
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
}
}
3nids
added a commit
to opengisch/QgisModelBaker
that referenced
this pull request
Oct 13, 2023
should bring better log messages see transifex/cli#157
This file contains hidden or 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
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.
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
shouldSkipDownloadto 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-percflag.Addresses #155