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

Logger abstraction #36

Merged
merged 10 commits into from Oct 2, 2022
Merged

Logger abstraction #36

merged 10 commits into from Oct 2, 2022

Conversation

JstnMcBrd
Copy link
Contributor

The logger was already partially abstracted before, but it required passing logger around through parameters in methods. This was causing problems for event handler abstraction.

Now the logger is in a separate file called logger.ts that contains a method that returns the logger to use (the console, for now). We use a method instead of a constant to make mocking easier for tests.

All files that used the logger before now pull the logger from logger.ts instead of passing through parameters. I also updated tests to reflect this fact by mocking logger.ts.

For now, just returns a reference to the console
Instead of passing console around, all files that need the logger pull it directly from logger.ts
Updated the tests for all changed files to mock the logger file
@JstnMcBrd JstnMcBrd added the enhancement New feature or request label Oct 1, 2022
@JstnMcBrd JstnMcBrd requested a review from a team October 1, 2022 22:35
@JstnMcBrd JstnMcBrd self-assigned this Oct 1, 2022
@github-actions
Copy link

github-actions bot commented Oct 1, 2022

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
95.81% (+0.29% 🔼)
389/406
🟢 Branches 87.93% 102/116
🟢 Functions
98.25% (+0.03% 🔼)
56/57
🟢 Lines
95.7% (+0.3% 🔼)
378/395
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢 logger.ts 100% 100% 100% 100%

Test suite run success

89 tests passing in 18 suites.

Report generated by 🧪jest coverage report action from a7e74a6

src/logger.ts Outdated Show resolved Hide resolved
Co-authored-by: James Robinson <1500092+AverageHelper@users.noreply.github.com>
AverageHelper
AverageHelper previously approved these changes Oct 2, 2022
@JstnMcBrd JstnMcBrd requested a review from a team October 2, 2022 00:29
@JstnMcBrd
Copy link
Contributor Author

I forgot some parts. I'll be pushing more changes soon.

AverageHelper
AverageHelper previously approved these changes Oct 2, 2022
gmacgre
gmacgre previously approved these changes Oct 2, 2022
Copy link
Contributor

@gmacgre gmacgre left a comment

Choose a reason for hiding this comment

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

Making a personal note here- we probably should have merged this in before xkcd. I'll need to go back and make sure that the logging there is up to date.

@JstnMcBrd
Copy link
Contributor Author

Since you just merged main into this branch, I can go and edit xkcd's code before merging.

@JstnMcBrd JstnMcBrd changed the title Logger abstraction Draft: Logger abstraction Oct 2, 2022
@JstnMcBrd JstnMcBrd changed the title Draft: Logger abstraction Logger abstraction Oct 2, 2022
@JstnMcBrd JstnMcBrd requested a review from gmacgre October 2, 2022 02:26
@JstnMcBrd JstnMcBrd merged commit 22e3bed into main Oct 2, 2022
@JstnMcBrd JstnMcBrd deleted the mcb/logger branch October 2, 2022 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants