-
Notifications
You must be signed in to change notification settings - Fork 36
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
Remove redundant pre-register StyledStrings compat #307
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #307 +/- ##
==========================================
+ Coverage 84.31% 85.60% +1.28%
==========================================
Files 8 9 +1
Lines 778 806 +28
==========================================
+ Hits 656 690 +34
+ Misses 122 116 -6 ☔ View full report in Codecov by Sentry. |
Note that there is also "test/Project.toml". |
b802707
to
f23896d
Compare
Hmmm, looks like I might need to make a patch release to the compat version of |
Yup, I'll do a 1.0.1 release shortly: see JuliaLang/StyledStrings.jl@dc94cfa |
I will merge PR #308 first to make it easier to see the CI results. (Edit: Merged) |
Cool, I'd thought I'd have 1.0.1 out by now, but there seems to be a holdup to do with the casing of |
This is off-topic and outside my scope of responsibility, but what is the review process and accountability of StyledStrings.jl? |
The way StyledStrings.jl is setup: Commits to Main go through PRs, always, even for small fixup commits that really don't need it. Commits to other branches (like Currently, I'm hoping that other people will get involved with this, but it's pretty much a just-me show ATM. Given that, the review process is "I think it looks good". |
37aa8f8
to
da7c643
Compare
The failing tests are from:
|
ColorTypes.jl/test/runtests.jl Lines 24 to 29 in e195457
Could we remove the try block?
|
I don't see why not, or why it was added in the first place? |
Because, as you know, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
(However, a separate decision on the release plan is needed.)
Now that StyledStrings is registered, we can do away with this code (and also include StyledStrings support in a release, which would be nice).