Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CO-357] Re-emit: Fix 'overrideConsoleLog' #3639

Merged
merged 1 commit into from
Sep 21, 2018
Merged

[CO-357] Re-emit: Fix 'overrideConsoleLog' #3639

merged 1 commit into from
Sep 21, 2018

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Sep 20, 2018 •
edited
Loading

Description

This was done in 7c59b1a but
then overriden by a wrong merge-conflict resolution in https://github.com/input-output-hk/cardano-sl/pull/3533/files#diff-f337564bf2743258dca7074867253d8fL241.

This commits re-applies the patch on top of the new changes.
Note that it turns out the 'Semigroup' instance for 'LoggerTree'
was broken :/ (being left-associative instead of right-associative).

Linked issue

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • [ ] CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • [ ] I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Screenshots (if available)

CodiePP
CodiePP suggested changes Sep 20, 2018
View reviewed changes
lib/src/Pos/Launcher/Resource.hs Outdated
-- add output to the console with severity filter >= Info
Just False -> identity
Just False -> src <> disabledConsoleConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disabledConsoleConfiguration should be a filter on the incoming LoggerConfig and remove all backends that write to the console.

util/src/Pos/Util/Log/LoggerConfig.hs Outdated
-- | Handler to override another console 'LogHandler' and disable logging
-- to console altogether.
disabledConsoleConfiguration :: LoggerConfig
disabledConsoleConfiguration = LoggerConfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just adds a 'no-op'; I don't think this was the intention.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this will collapse with other LoggerConfig called "console". I thought that's how logging to console was identified.
I am adapting this and simply filtering out StdOutBE and StdErrBE 👍

util/src/Pos/Util/Log/LoggerConfig.hs Outdated
@@ -274,6 +275,25 @@ defaultInteractiveConfiguration minSeverity =
in
LoggerConfig{..}

-- | Handler to override another console 'LogHandler' and disable logging
-- to console altogether.
disabledConsoleConfiguration :: LoggerConfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will write a test to demonstrate this..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@KtorZ KtorZ force-pushed the KtorZ/CO-390/refix-override-console-log branch from 0ace1a1 to b6fc657 Compare September 21, 2018 07:28
CodiePP
CodiePP approved these changes Sep 21, 2018
View reviewed changes
Copy link
Contributor

@CodiePP CodiePP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

lib/src/Pos/Launcher/Resource.hs Outdated
-- add output to the console with severity filter >= Info
Just False -> identity
Just False -> lcLoggerTree . ltHandlers %~ filter (not . isWritingToConsole)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pedantic, there is a superfluous space.
LGTM! I like your solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho right :o ... I'll remove 👍

@CodiePP
Copy link
Contributor

CodiePP commented Sep 21, 2018

hlint complains:

util/src/Pos/Util/Log/LoggerConfig.hs:131:5: Warning: Use elem
Found:
  any (== (_lhBackend lh))
Why not:
  elem (_lhBackend lh)

@KtorZ
Copy link
Contributor Author

KtorZ commented Sep 21, 2018

hlint complains:

For good reasons 😅

@KtorZ
Copy link
Contributor Author

KtorZ commented Sep 21, 2018

Also, interesting failure in Hydra:


  test/Test/Pos/Util/TimerSpec.hs:72:5: 
  1) Test.Pos.Util.Timer.Timer should be additive
       Assertion failed (after 4 tests):
         266mcs
         127mcs

Randomized with seed 1182359330

@CodiePP Does that ring a bell?

@KtorZ KtorZ force-pushed the KtorZ/CO-390/refix-override-console-log branch from b6fc657 to db94614 Compare September 21, 2018 07:46
@KtorZ
[CO-357] Re-emit: Fix 'overrideConsoleLog'
daf029e 
This was done in 7c59b1a but
then overriden by a wrong merge-conflict resolution in f337564bf2743258dca7074867253d8f.

This commits re-applies the patch on top of the new changes.
Note that it turns out the  'Semigroup' instance for 'LoggerTree'
was broken :/ (being left-associative instead of right-associative).
@KtorZ KtorZ force-pushed the KtorZ/CO-390/refix-override-console-log branch from db94614 to daf029e Compare September 21, 2018 08:15
@KtorZ KtorZ merged commit d2af290 into develop Sep 21, 2018
@KtorZ KtorZ deleted the KtorZ/CO-390/refix-override-console-log branch September 21, 2018 13:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CodiePP CodiePP CodiePP approved these changes

@erikd erikd Awaiting requested review from erikd

@365andreas 365andreas Awaiting requested review from 365andreas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KtorZ @CodiePP