Open Bug 1281570 Opened 3 years ago Updated 11 months ago

Provide recommendations to fix typos when using `mach try`

Categories

(Firefox Build System :: Try, defect)

defect
Not set

Tracking

(firefox50 fixed)

REOPENED
Tracking Status
firefox50 --- fixed

People

(Reporter: jaws, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Since bug 1258451 we now will validate whether some of the options are correct, but we print out a big list of all of the options.

It would be better if we only printed options that were close to what was requested.

For example:

./mach try --no-push -p win -u mochitest
Invalid choice 'win'. Did you mean ['win32', 'win64]?
Invalid choice 'mochitest'. Did you mean ['mochitests', 'mochitest-1', 'mochitest-2', 'mochitest-3', 'mochitest-4', 'mochitest-5', 'mochitest-6', 'mochitest-7', 'mochitest-8', 'mochitest-9']?
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8764370 - Flags: review?(cmanchester)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8764370 - Attachment is obsolete: true
Attachment #8764370 - Flags: review?(cmanchester)
Attachment #8764372 - Flags: review?(cmanchester)
Comment on attachment 8764372 [details] [diff] [review]
Patch v1.1

Review of attachment 8764372 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/tools/autotry/autotry.py
@@ +14,5 @@
>  
>  import ConfigParser
>  
> +# Implementation based on pseudo-code from Wikipedia,
> +# https://en.wikipedia.org/wiki/Levenshtein_distance#Iterative_with_two_matrix_rows

I see we use difflib from the Python standard library in other places around the tree, would a function from that library be appropriate here?

@@ +58,5 @@
> +            if distance < 3L:
> +                corrections.append(choice)
> +        if len(corrections) > 0:
> +            valid = False
> +            print 'Invalid choice {v!r}. Did you mean: {c!r}?'.format(v=value, c=corrections)

This has an interesting effect: near misses are rejected, but everything else is accepted.

This actually seems like a decent behavior in this case (although I guess it would be problematic in the case we did something like add chunks).
Attachment #8764372 - Flags: review?(cmanchester)
Attached patch Patch v2 (obsolete) — Splinter Review
Thanks, this is much better.
Attachment #8764372 - Attachment is obsolete: true
Attachment #8764584 - Flags: review?(cmanchester)
Attached patch Patch v2.1Splinter Review
Actually, since the v1 and v2 patch allowed non-similar errors to pass through it regressed bug 1277127.

This version of the patch will now report when there are no similar matches and ask the user if they are sure they want to proceed.

I also fixed the placement of the 'under development' warning to appear before any parsing validation has run.
Attachment #8764584 - Attachment is obsolete: true
Attachment #8764584 - Flags: review?(cmanchester)
Attachment #8764594 - Flags: review?(cmanchester)
Comment on attachment 8764594 [details] [diff] [review]
Patch v2.1

Review of attachment 8764594 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/tools/autotry/autotry.py
@@ +20,4 @@
>      for value in values:
> +        corrections = difflib.get_close_matches(value, choices)
> +        if len(corrections) == 0:
> +            print 'Potentially invalid choice {v!r}. Requested value nor similar values are not found in list of possible values.'.format(v=value)

This sentence read a little easier as "Neither the requested value nor a similar value..."

@@ +28,5 @@
> +        elif corrections[0] == value:
> +            continue
> +        else:
> +            valid = False
> +            print 'Invalid choice {v!r}. Some suggestions (limit three): {c!r}?'.format(v=value, c=corrections)

Do you need to pass 3 to get_close_matches to actually limit the results?
Attachment #8764594 - Flags: review?(cmanchester) → review+
(In reply to Chris Manchester (:chmanchester) from comment #6)
> > +        elif corrections[0] == value:
> > +            continue
> > +        else:
> > +            valid = False
> > +            print 'Invalid choice {v!r}. Some suggestions (limit three): {c!r}?'.format(v=value, c=corrections)
> 
> Do you need to pass 3 to get_close_matches to actually limit the results?

No, because 3 is the default value used by get_close_matches. https://docs.python.org/2/library/difflib.html
https://hg.mozilla.org/integration/fx-team/rev/efec1285a2a18a1fef1c6242b22d72638e088330
Bug 1281570 - Provide recommendations to fix typos when using 'mach try'. r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/efec1285a2a1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Backed this out as part of the backout for bug 1258451 in

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea448e55b6426a2afc24e29c38e66813a6dc0c70
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: jaws → nobody
Component: General → Try
Product: Testing → Firefox Build System
Target Milestone: mozilla50 → ---
Version: Version 3 → unspecified
You need to log in before you can comment on or make changes to this bug.