migrate the build command to use the build daemon #298
Conversation
grouma
left a comment
grouma
left a comment
There was a problem hiding this comment.
I'm surprised there are no test changes. They all work as expected?
I was just letting travis find the errors and ill fix them up - but there don't seem to be a lot, mostly logging related. |
grouma
left a comment
grouma
left a comment
There was a problem hiding this comment.
LGTM. Ping me when you have fixed the tests.
| 'directory.')); | ||
|
|
||
| await process.shouldExit(73); | ||
| await process.shouldExit(isNot(0)); |
There was a problem hiding this comment.
Is this technically a breaking change?
There was a problem hiding this comment.
This was potentially valuable, though I don't know if there were any actual users. Is it possible to still support it?
There was a problem hiding this comment.
We can't reasonably support it as it stands right now I don't think, we don't get exit codes from the build daemon as far as I could tell.
|
Do we want to get this in for v2? |
| var log = serverLog.log; | ||
| Level recordLevel; | ||
| for (var level in Level.LEVELS) { | ||
| if (log.startsWith('[$level]')) { |
There was a problem hiding this comment.
We're going to end up building this string over and over. I wonder if we want something like:
final levelsByLabel = Map.fromIterable(Level.LEVELS, key: (l) => '[$l]'); // should be static
...
var messageLabel = levelsByLabel.keys.firstWhere(log.startsWith);
return messageLabel == null ? null : levelsByLabel[messageLabel];Then we can look forward to writing it instead as
var levelsByLabel = {for (var l in Level.LEVELS) '[$l]': l};There was a problem hiding this comment.
I just moved these utilities around in this pr since they aren't specific to the serve command any more, we could follow up with something but I think we have a lot of other much slower things going on in the logging than this :D.
Also probably a switch statement or something would probably be faster than a map for that if we are concerned about perf?
yes |
4 participants
No description provided.