Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove date from logger #333

Merged
merged 9 commits into from Jan 24, 2015
Merged

Remove date from logger #333

merged 9 commits into from Jan 24, 2015

Conversation

pascalduez
Copy link
Member

So [18:26:48] instead of superfluous [2015-01-22 18:26:48]

Thoughts ?

@valeriangalliat
Copy link
Member

The date was not needed indeed, neither is the time IMO.

@pascalduez
Copy link
Member Author

The date was not needed indeed, neither is the time IMO.

Time kind of make sense in a debug scenario, where one could merely check execution time... ?
But usually SassDoc performs in less than a second.

@pascalduez
Copy link
Member Author

Maybe the normal (non debug) verbose mode should rather looks like something in this vein ?

[18:45:09] SassDoc task started
> Item "global-test" refers to "mixin-global-test" from type "mixin" but this item doesn't exist.
> Item "global-test" refers to "variable-global-test" from type "variable" but this item doesn't exist.
> Item "global-test" refers to "placeholder-global-test" from type "placeholder" but this item doesn't exist.
> Folder "./test/data" successfully parsed.
> Folder "sassdoc" successfully refreshed.
> Theme "default" successfully rendered.
> Process over. Everything okay!
[18:45:09] SassDoc task ended

@KittyGiraudel
Copy link
Member

Maybe the normal (non debug) verbose mode should rather looks like something in this vein ?

Looks nice.

@valeriangalliat
Copy link
Member

where one could merely check execution time

time sassdoc ...

@pascalduez
Copy link
Member Author

So shall we go for the fancier formatting ? Or just keep it as is ?

» Item "global-test" refers to "mixin-global-test" from type "mixin" but this item doesn't exist.
» Item "global-test" refers to "variable-global-test" from type "variable" but this item doesn't exist.
» Item "global-test" refers to "placeholder-global-test" from type "placeholder" but this item doesn't exist.
» Folder "./test/data" successfully parsed.
» Folder "sassdoc" successfully refreshed.
» Theme "default" successfully rendered.
» Process over. Everything okay!
✔ SassDoc task ended in 200ms

@KittyGiraudel
Copy link
Member

Could we have an octopus icon? :x

@KittyGiraudel
Copy link
Member

@pascalduez
Copy link
Member Author

Current status:
screen shot 2015-01-23 at 23 08 28

Still room for improvements.

@FWeinb
Copy link
Member

FWeinb commented Jan 23, 2015

I like it ✔

@KittyGiraudel
Copy link
Member

👍

@valeriangalliat
Copy link
Member

Looks good!

@pascalduez
Copy link
Member Author

screen shot 2015-01-24 at 11 20 23

@valeriangalliat
Copy link
Member

Errors have a red » too?

@pascalduez
Copy link
Member Author

Those are just 'log' messages, I kept the [WARNING] and [ERROR] ones we already had.
Should we streamline all messages ?

@FWeinb
Copy link
Member

FWeinb commented Jan 24, 2015

Making it red on an error and yellow on a warning sound awesome

@valeriangalliat
Copy link
Member

Should we streamline all messages ?

I think so, but I don't know how. It's nice to differenciate heads-up from success with yellow/green but if we add this scheme for warning messages, they'd be yellow too…

Though "but this item doesn't exist" messages are kinda warnings to me.

@valeriangalliat
Copy link
Member

I think we need to change the logger semantics (and function names):

function color
log (same as info) default
success green
warning yellow
error red

@pascalduez
Copy link
Member Author

Looks good. So I would also put the whole line colored for warning and error.
Should we keep the [WARNING] and [ERROR] prepends ?

» [WARNING] You'd better watch out bro, something's broken.
» [ERROR] You're screwed anyway.

I give it a try so we can check how it looks.

@valeriangalliat
Copy link
Member

Visually, with colors, we don't need [WARNING] and [ERROR], though it's really helpful when logs are saved to text files to search through them. it's the only way on non-colored terminals to differenciate informational messages from warnings and errors.

An alternative would be to replace the » with I W or E for respectively info, warning, error (in the corresponding color).

I don't think the whole line needs to be colored, only the » or "semantic leter" is enough.

@FWeinb
Copy link
Member

FWeinb commented Jan 24, 2015

I would include [WARNING] and [ERROR] and only color the chevron.

@pascalduez
Copy link
Member Author

Okay for only having the prepended thing colored.

Although I like the I W or E I don't think it's straightforward enough for end users ?

@pascalduez
Copy link
Member Author

I would include [WARNING] and [ERROR] and only color the chevron.

👍 let's try

@valeriangalliat
Copy link
Member

I don't know, I recall seeing this in unit test suites. Having the [] is okay too.

@FWeinb
Copy link
Member

FWeinb commented Jan 24, 2015

Let's look at the output of popular unit test frameworks

@pascalduez
Copy link
Member Author

I don't know, I recall seeing this in unit test suites. Having the [] is okay too.

What I meant is it's maybe a bit too nerdy :-) We have devsigner™ between SassDoc users.

@pascalduez
Copy link
Member Author

screen shot 2015-01-24 at 12 21 27

screen shot 2015-01-24 at 12 23 53
Stacktrace truncated from screenshot

@FWeinb
Copy link
Member

FWeinb commented Jan 24, 2015

This looks awesome! But just name it SassDoc completed after xxxms

@valeriangalliat
Copy link
Member

Perfect!

@pascalduez
Copy link
Member Author

And the logger code has been considerably lightened/cleaned.

Also now that annotations use warn instead of log, the warnings appears in the tests.
But I guess it even better, warnings should be considered, not ignored.

@pascalduez
Copy link
Member Author

Mr @hugogiraudel the last word maybe ?

@KittyGiraudel
Copy link
Member

You guys rock. LGTM.

Le Sam 24 Jan 2015 12:34, Pascal Duez notifications@github.com a écrit :

Mr @hugogiraudel https://github.com/HugoGiraudel the last word maybe ?


Reply to this email directly or view it on GitHub
#333 (comment).

pascalduez added a commit that referenced this pull request Jan 24, 2015
Streamline and simplify logger messages
@pascalduez pascalduez merged commit 293902b into develop Jan 24, 2015
@pascalduez pascalduez deleted the simplify-logger branch January 24, 2015 12:24
@pascalduez
Copy link
Member Author

Thanks team for the great and constructive feedback !

@KittyGiraudel
Copy link
Member

Tried it today, looks gorgeous. Love it.

@pascalduez
Copy link
Member Author

And its now fully tested / testable.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bc8957b on simplify-logger into * on develop*.

@pascalduez
Copy link
Member Author

@coveralls U drunk?

@valeriangalliat
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants