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

Use StyledStrings for Logging #51829

Merged
merged 1 commit into from Feb 10, 2024
Merged

Conversation

tecosaur
Copy link
Contributor

@tecosaur tecosaur commented Oct 23, 2023

Transition from printstyled to the new approach to styling provided by StyledStrings.

Along the way, this makes it possible to restyle logging should you desire to do so (for whatever reason), but also means that you can inherit the colours used in logging in other places.

10-24-00:10

This requires JuliaLang/StyledStrings.jl#3 and JuliaLang/StyledStrings.jl#6 to be resolved first.

In the meantime, a bikeshedding opportunity: log.debug (et al.) vs. logging.debug (et al.)?

@tecosaur tecosaur added domain:display and printing Aesthetics and correctness of printed representations of objects. logging The logging framework labels Oct 23, 2023
@tecosaur
Copy link
Contributor Author

Good to merge after #51869.

@tecosaur
Copy link
Contributor Author

Fixed the merge conflict introduced in #52196.

@tecosaur
Copy link
Contributor Author

We could invoke StyledStrings.legacy_color to make @create_log_macro support colour numbers and old names, but since it's new API I'm not sure that this is needed.

@tecosaur
Copy link
Contributor Author

I'm still interested in thoughts on

log.debug (et al.) vs. logging.debug (et al.)?

since I'm on the fence myself

vtjnash added a commit that referenced this pull request Feb 1, 2024
This has some value by itself, but mainly serves as a prerequisite for
#51811, #51816, and #51829, along with two other PRs that have yet to be
made.
@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 1, 2024

Other than the aforementioned bikeshed, I think this is just a review short of merging?

@tecosaur tecosaur added the status:awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Feb 1, 2024
@tecosaur tecosaur force-pushed the logging-with-style branch 2 times, most recently from ab03c94 to 0c0c385 Compare February 4, 2024 15:25
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Aside from needing to fix some tests, this LGTM

@vtjnash vtjnash removed the status:awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Feb 5, 2024
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 5, 2024

You have triage permissions, so you can tag your PRs with "merge me" whenever you get them to a point that is ready and with tests passing (which is slightly more prompt at getting noticed than the "awaiting review" tag)

@tecosaur tecosaur force-pushed the logging-with-style branch 2 times, most recently from da5e9e1 to 4e38ba7 Compare February 7, 2024 20:27
@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 7, 2024

  • I'll bear merge me, in mind, I do like getting someone else's eyes on my code before merging though 🙂
  • I've just pushed an updated runtests.jl for Logging that should be happy now 🤞
  • I noticed that I forgot to update pkgimage.mk, and so have pushed an update to it too

@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 8, 2024

Looking at the test state, it seems like we're good to go? Pkg isn't happy, but that seems to be that it has a bunch of test manifests that don't include StyledStrings as a dep of Logging, and they need to be updated.

I suspect I'm not the best person to take care of this Pkg-side issue.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 8, 2024

I think that test was recently deleted from Pkg due to other bugs in that precompile test. But there is a test that you will need to update (it is a list of dependencies of things in some packages, for testing purposes that it can read those lists):

precompile                                        (1) |         failed at 2024-02-07T23:42:02.391
2024-02-07 18:42:02 EST	Test Failed at /cache/build/tester-amdci4-12/julialang/julia-master/julia-4e38ba77bc/share/julia/test/precompile.jl:475
2024-02-07 18:42:02 EST	  Expression: Dict(modules) == modules_ok

@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 8, 2024

Ah right, thanks. I think I've adjusted modules_ok appropriately, we'll see how that goes 🤞.

Transition from printstyled to the new approach to styling provided by
StyledStrings.

This both makes it possible for the styling used to be customised, and
allows for other styled content to inherit/re-use the logging styles.
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 8, 2024
@vtjnash vtjnash merged commit 250916f into JuliaLang:master Feb 10, 2024
6 of 8 checks passed
@oscardssmith oscardssmith removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 10, 2024
KristofferC pushed a commit that referenced this pull request Feb 15, 2024
Can be seen as a follow-on from #51829, also wants
JuliaLang/StyledStrings.jl#37
 to behave as expected.
tecosaur added a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
KristofferC pushed a commit that referenced this pull request May 10, 2024
KristofferC pushed a commit that referenced this pull request May 10, 2024
KristofferC pushed a commit that referenced this pull request May 13, 2024
aviatesk pushed a commit that referenced this pull request May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:display and printing Aesthetics and correctness of printed representations of objects. logging The logging framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants