Add an optional callback to @checking

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8734559 [details]
MozReview Request: Bug 1259620 - Add an optional formatting callback to @checking. r?gps

Review commit: https://reviewboard.mozilla.org/r/42359/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42359/
Attachment #8734559 - Flags: review?(gps)
(Assignee)

Comment 2

3 years ago
Created attachment 8734560 [details]
MozReview Request: Bug 1259620 - Add @checking to host and target to display the triplets. r?gps

Review commit: https://reviewboard.mozilla.org/r/42361/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42361/
Attachment #8734560 - Flags: review?(gps)

Comment 3

3 years ago
Comment on attachment 8734559 [details]
MozReview Request: Bug 1259620 - Add an optional formatting callback to @checking. r?gps

https://reviewboard.mozilla.org/r/42359/#review38931

::: build/moz.configure/checks.configure:27
(Diff revision 1)
>  #   def check(value):
>  #       ...
> +# An optional callback can be given, that will be used to format the returned
> +# value when displaying it.
>  @template
> -def checking(what):
> +def checking(what, callback=None):

`callback` feels a bit generic to me. But I suppose we can mass rename it later. It's not like we have to maintain API compatibility.

::: build/moz.configure/checks.configure:63
(Diff revision 1)
>      if not (isinstance(progs, tuple) or isinstance(progs, list)):
>          configure_error('progs should be a list or tuple!')
>      progs = list(progs)
>  
>      @depends(var)
> -    @checking('for %s' % var.lower())
> +    @checking('for %s' % var.lower(), lambda x: x or 'not found')

Please use a named argument.
Attachment #8734559 - Flags: review?(gps) → review+

Comment 4

3 years ago
Comment on attachment 8734560 [details]
MozReview Request: Bug 1259620 - Add @checking to host and target to display the triplets. r?gps

https://reviewboard.mozilla.org/r/42361/#review38933
Attachment #8734560 - Flags: review?(gps) → review+
(Assignee)

Comment 5

3 years ago
(In reply to Gregory Szorc [:gps] from comment #3)
> Please use a named argument.

Why? If we're not going to change the name, callback is kind of redundant with the fact it's a lambda. As for a better name than callback, I don't have an idea that would be short. A long name would also add noise on the call site.
Flags: needinfo?(gps)

Comment 6

3 years ago
Fine, land it as is.
Flags: needinfo?(gps)

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/67e1d5126c9f
https://hg.mozilla.org/mozilla-central/rev/a7d69f146a52
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.