If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[mozlint] Improve color handling in stylish formatter

RESOLVED FIXED in Firefox 51

Status

Testing
Lint
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

Version 3
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
I want to change the 'grey' color to 247 to match the eslint formatter's color. But I also want to support terminals with less than 256 colors. We should improve the color handling in the stylish formatter to provide fallback values.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
Comment on attachment 8780641 [details]
Bug 1294802 - Improve color handling in mozlint stylish formatter,

https://reviewboard.mozilla.org/r/71310/#review70640

::: python/mozlint/mozlint/formatters/stylish.py:53
(Diff revision 2)
>      def __init__(self, disable_colors=None):
>          if disable_colors or not blessings:
>              self.term = NullTerminal()
>          else:
>              self.term = blessings.Terminal()
> +        self.num_colors = self.term.number_of_colors

For what it's worth, I would probably cache the right color settings for the terminal at this point i.e.

    def _first_lt(lst, limit):
        for item in lst:
            if item < limit:
                return item
        return 0

    self.color = {colour: _first_lt(values, self.num_colors) for color, value in self._colors.iteritems()}
    
and then

    self.color['grey']
    
But it's only worth changing if you think that the performance win is worthwhile.
Attachment #8780641 - Flags: review?(james) → review+
(Assignee)

Comment 4

a year ago
mozreview-review-reply
Comment on attachment 8780641 [details]
Bug 1294802 - Improve color handling in mozlint stylish formatter,

https://reviewboard.mozilla.org/r/71310/#review70640

> For what it's worth, I would probably cache the right color settings for the terminal at this point i.e.
> 
>     def _first_lt(lst, limit):
>         for item in lst:
>             if item < limit:
>                 return item
>         return 0
> 
>     self.color = {colour: _first_lt(values, self.num_colors) for color, value in self._colors.iteritems()}
>     
> and then
> 
>     self.color['grey']
>     
> But it's only worth changing if you think that the performance win is worthwhile.

I agree this is better. But the performance cost is probably negligible, the autoland button is looking very tempting, and I am a lazy man.. :)

Comment 5

a year ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1cc4e06979a
Improve color handling in mozlint stylish formatter, r=jgraham

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1cc4e06979a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.