add colors to logs from ANSI control chars. #1103
Conversation
snatchev
changed the title
lib/ansi.rb
Outdated
| @@ -0,0 +1,62 @@ | |||
| module FastlaneCI | |||
| ## | |||
| # little helper class for parsing ansi to html | |||
There was a problem hiding this comment.
Did you write this class, or is it taken from somewhere?
There was a problem hiding this comment.
It's original content :)
snatchev
changed the title
|
Can we move this Ansi color library to the front-end? I think we should avoid html injection when possible. |
|
@nakhbari I am in favor of moving it to the front-end. But I didn't know where to start, so I didn't do it there. |
|
@snatchev I'm more than happy to pair on this. I think it would take half a day. |
KrauseFx
added
the
status: blocked
|
Talking with @snatchev, not merging this until things are moved to frontend |
|
Related GH issue #1054 |
snatchev
removed
the
status: blocked
| @ViewChild('logLine', { read: ElementRef }) logLineEl: ElementRef; | ||
|
|
||
| ngAfterViewInit() { | ||
| const parentEl = this.logLineEl.nativeElement; |
There was a problem hiding this comment.
can we encapsulate the work being done in this method into a function? It's easier to following what's being done after this lifecycle hook is called.
| let currentSpanEl = parentEl; | ||
| const stack = this.tokenize(this.log.message); | ||
|
|
||
| stack.forEach(tuple => { |
There was a problem hiding this comment.
please use a for-of loop instead
for (const ansiSection of ansiSectionsStack) {
...
}
|
|
||
| stack.forEach(tuple => { | ||
| const [code, text] = tuple; | ||
| if (code === '0') { |
There was a problem hiding this comment.
could you explain what code 0 is here? or maybe make it a constant like const ANSI_RESET_CODE = '0';
| * this method will split and group the result into an array of tuples [ansi code, text] | ||
| * NOTE: that the capture comes after the string, so we transpose them. | ||
| **/ | ||
| private tokenize(text: string): [string, string][] { |
There was a problem hiding this comment.
totally optional, but you can create a type alias for ansi code to make the typings mean more than string.
ex.
type AnsiCode = string;
private tokenize(text: string): [AnsiCode, string][] { ... }
| if (tuples[0] === '') { | ||
| tuples.shift(); | ||
| } | ||
| // otherwise, we must assume we are starting each line as the default '0' |
There was a problem hiding this comment.
can you put the comment inside the block or alongside the previous comment?
| } | ||
|
|
||
| private injectSpan(parent: HTMLElement, text: string, ansiCode: string): HTMLSpanElement { | ||
| const span = document.createElement('span'); |
There was a problem hiding this comment.
can you provide the document in the constructor. We should be using injectables instead of the direct global var
ex. https://github.com/fastlane/ci/blob/master/web/app/onboard/onboard.component.ts#L29
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
can we add the background + color styles here?
i think it's this:
padding: 8px;
color: #93a1a1;
background-color: #073642;
font-family: 'Google Sans Monospace', monospace;
| fixture.detectChanges(); | ||
| }); | ||
|
|
||
| it('should create', () => { |
There was a problem hiding this comment.
let's add a test to ensure that the loglines are rendered.
| const stack = this.tokenize(this.log.message); | ||
|
|
||
| for (const tuple of stack) { | ||
| const [code, text] = tuple; |
There was a problem hiding this comment.
you can actually merge the previous two lines
for (const [ansiCode, text] of stack) { ... }
There was a problem hiding this comment.
I tried... Typescript complained at me.
rspecand corrected all errorsrubocopand corrected all errorsnpm run testand corrected all errorsThis PR adds colors to the logs by parsing out ANSI control chars:
