Skip to content

Optional logger refactor#14

Closed
walro wants to merge 3 commits intomasterfrom
optional-logger-refactor
Closed

Optional logger refactor#14
walro wants to merge 3 commits intomasterfrom
optional-logger-refactor

Conversation

@walro
Copy link
Contributor

@walro walro commented Dec 1, 2020

Based on #13

The difference between this and #13 is:

  • Slimmer diff compared to master when it comes to the specs
  • Refactor specs to emphasize on the logger rather than the constructor (not 100% sure this is 100% better though, as the other specs are put on method level, now we have a higher-level of description here, sort of)

Pontus4 and others added 3 commits December 1, 2020 10:53
...and add corresponding spec

Close #9
...since it is not needed.
@walro
Copy link
Contributor Author

walro commented Dec 1, 2020

I was thinking if you agree with things here you could "pull the changes" in to #13 and we'll close this one

@walro walro marked this pull request as draft December 1, 2020 12:58
@Pontus4
Copy link
Contributor

Pontus4 commented Dec 1, 2020

  • Slimmer diff compared to master when it comes to the specs

It is probably a good idea to simplify the diff a bit!

  • Refactor specs to emphasize on the logger rather than the constructor (not 100% sure this is 100% better though, as the other specs are put on method level, now we have a higher-level of description here, sort of)

I'm not sure which option is better here. Like you say I chose to put the tests under #initialize to fit in with the rest, and if there is no clear "winner" I lean towards keeping it that way :)

@walro
Copy link
Contributor Author

walro commented Dec 1, 2020

Like you say I chose to put the tests under #initialize to fit in with the rest, and if there is no clear "winner" I lean towards keeping it that way :)

Sure

@walro walro closed this Dec 1, 2020
@walro walro deleted the optional-logger-refactor branch December 1, 2020 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants