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.

Sign up for GitHub

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

Jump to bottom

cleanup/colors-package-removal #541

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

cleanup/colors-package-removal #541

wants to merge 6 commits into from

Conversation

jszuminski
Copy link
Contributor

@jszuminski jszuminski commented Jul 16, 2024 •
edited
Loading

What and why?

The goal of this pull request is to get rid of the unnecessary colors dependency and perform a slight refactoring of the logging logic. The logging statements (even the longer ones) were shared across multiple files even though they have been exactly the same. If someone wanted to modify them in the future, he or she would have to modify 8 files (which might get tricky).

Tasks

  • remove colors package
  • replace all its appearances with our custom logging colors
  • add this change to changelog
  • slight cleaning of the logger.js and most console log statements

@jszuminski jszuminski self-assigned this Jul 16, 2024
@jszuminski
Copy link
Contributor Author

@PaulDalek what do you think about this type of defining colors and styles? Is it okay here in this use case or should we handle them differently?

bvogel
bvogel reviewed Jul 17, 2024
View reviewed changes
Copy link

@bvogel bvogel left a comment

Choose a reason for hiding this comment

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

only minor stylistc changes

lib/utils.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
tests/node/node_test_runner_single.js Outdated Show resolved Hide resolved
tests/node/node_test_runner_single.js Outdated Show resolved Hide resolved
tests/other/private_range_url.js Outdated Show resolved Hide resolved
tests/other/stress_test.js Outdated Show resolved Hide resolved
tests/test_utils.js Outdated Show resolved Hide resolved
tests/test_utils.js Outdated Show resolved Hide resolved
tests/test_utils.js Outdated Show resolved Hide resolved
tests/test_utils.js Outdated Show resolved Hide resolved
jszuminski and others added 4 commits July 17, 2024 09:55
@jszuminski @bvogel
Update tests/test_utils.js
0429e3f 
Co-authored-by: Burkhard Vogel-Kreykenbohm <[email protected]>
@jszuminski @bvogel
Update tests/test_utils.js
3948d6b 
Co-authored-by: Burkhard Vogel-Kreykenbohm <[email protected]>
@jszuminski @bvogel
Update tests/test_utils.js
26e86ec 
Co-authored-by: Burkhard Vogel-Kreykenbohm <[email protected]>
@jszuminski @bvogel
Apply suggestions from code review
adb9caf 
Co-authored-by: Burkhard Vogel-Kreykenbohm <[email protected]>
@jszuminski
Copy link
Contributor Author

Thanks @bvogel! Great suggestions, all applied.

@bvogel
Copy link

bvogel commented Jul 18, 2024

Glad you liked them, hopefully this get's merged and released soon!

@jszuminski jszuminski marked this pull request as draft July 22, 2024 12:03
@bvogel
Copy link

bvogel commented Aug 5, 2024

too bad, didn't make it into 4.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bvogel bvogel bvogel left review comments

@PaulDalek PaulDalek Awaiting requested review from PaulDalek

At least 1 approving review is required to merge this pull request.

Assignees

@jszuminski jszuminski

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jszuminski @bvogel