Snapcraft

Merge lp://qastaging/~elopio/snapcraft/log_handler-2 into lp://qastaging/~snappy-dev/snapcraft/core

  1. log_handler-2
  2. Merge into core
Proposed by Leo Arias
Status: Needs review
Proposed branch: lp://qastaging/~elopio/snapcraft/log_handler-2
Merge into: lp://qastaging/~snappy-dev/snapcraft/core
Prerequisite: lp://qastaging/~elopio/snapcraft/log_handler
Diff against target: 105 lines (+42/-9)
4 files modified
snapcraft/cmds.py (+1/-3)
snapcraft/log.py (+26/-5)
snapcraft/tests/test_cmds.py (+2/-1)
snapcraft/tests/test_log.py (+13/-0)
To merge this branch: bzr merge lp://qastaging/~elopio/snapcraft/log_handler-2
Reviewer Review Type Date Requested Status
Snappy Developers Pending
Review via email: mp+266481@code.qastaging.launchpad.net

Commit message

Added filters to better handle the bold in the log.

Description of the change

Here again I went the extra mile to keep the existing behaviour. But this stuff opens the way to fancier and consistent logging, so we can change it whenever we like.

To post a comment you must log in.
Revision history for this message
Michael Terry (mterry) wrote :

This handles the specific case of the "Wrote the following as snapcraft.yaml:" message. But it feels a bit too magic to me. Doesn't it feel weird to automatically bold things based on whether a colon-newline appears in the string?

This also doesn't address the other print() statements (like when we run a subcommand.

Wouldn't it make more sense to have an argument to the info() call (maybe provided by a subclassed Logger class from snapcraft.log)?

Reply
Revision history for this message
Leo Arias (elopio) wrote :

> Wouldn't it make more sense to have an argument to the info() call (maybe
> provided by a subclassed Logger class from snapcraft.log)?

That was my first approach, extending the Format class. But then it breaks the original behaviour, where all the args and kwars passed will be used to create the string. Like:
logger.info('test {} {var}', 'v1', var='v2')

If you prefer handling the arguments ourselfs, I can easily revert.

Reply
Revision history for this message
Leo Arias (elopio) wrote :

> This also doesn't address the other print() statements (like when we run a
> subcommand.

That's the following branch. Looking for some free time to do it.
What I have in mind for that is to have a separate logger, named like __name__ + '.plugin'
Using a namespace, I can easily set a different format for it, without the bold.

Reply
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

what is the gain here?

Reply
Revision history for this message
Leo Arias (elopio) wrote :

By putting it into a filter then all the logs we send will follow the same style. The alternatives require the caller to know too much about the formatting specifics, which will result in inconsistencies.

Reply
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

I'll take another look at this if you merge with trunk again.

Reply

Unmerged revisions

116. By Leo Arias

Added filters to better handle the bold in the log.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: