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

fix: Apply ISP to improve code flexibility. #17

Merged
merged 1 commit into from Dec 31, 2018

Conversation

unional
Copy link
Contributor

@unional unional commented Dec 29, 2018

Change the logger type from Logger to simply { id: string }.

This is what's need by the ConsoleAppender and reduce the unnecessary coupling between the Appender and the Logger.

This allows the Appender to improve and use independently of the Logger.

This change follows the Interface Segregation Principle to declare and consume only what you need.

As such, the aurelia-logging dependency is moved to devDependency.

@EisenbergEffect
Copy link
Contributor

Thanks @unional Makes good sense to me 😄 Can you amend this PR by removing the dist folder? We'll generate that when we build/release but generally don't want to include it in normal PRs. Thanks!

@unional
Copy link
Contributor Author

unional commented Dec 30, 2018

Done. 🌷

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Dec 31, 2018

Oops! I'm really sorry @unional! I explained things incorrectly. I didn't mean to have you remove the dist folder, but rather to remove your changes to the dist folder. In other words, don't commit anything from the dist folder. Sorry for the confusion and the hassle! Do you think you can make that change?

@unional
Copy link
Contributor Author

unional commented Dec 31, 2018

I see. Force pushed the change. 🌮

@EisenbergEffect EisenbergEffect merged commit 372bf5c into aurelia:master Dec 31, 2018
@EisenbergEffect
Copy link
Contributor

Excellent. Thanks for your patience!

@unional unional deleted the ISP branch December 31, 2018 05:51
@unional
Copy link
Contributor Author

unional commented Dec 31, 2018

No problem at all. Thanks for taking the PR. 🌷

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.

None yet

2 participants