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

Remove redundant pre-register StyledStrings compat #307

Merged
merged 1 commit into from
May 9, 2024

Conversation

tecosaur
Copy link
Contributor

@tecosaur tecosaur commented May 6, 2024

Now that StyledStrings is registered, we can do away with this code (and also include StyledStrings support in a release, which would be nice).

Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.60%. Comparing base (142353d) to head (3a8f027).
Report is 9 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@kimikage kimikage marked this pull request as draft May 6, 2024 12:01
@kimikage
Copy link
Collaborator

kimikage commented May 6, 2024

Note that there is also "test/Project.toml".

@tecosaur tecosaur force-pushed the master branch 2 times, most recently from b802707 to f23896d Compare May 6, 2024 14:01
@tecosaur
Copy link
Contributor Author

tecosaur commented May 6, 2024

Hmmm, looks like I might need to make a patch release to the compat version of StyledStrings to improve the behaviour when precompiled within another package.

@tecosaur
Copy link
Contributor Author

tecosaur commented May 6, 2024

Yup, I'll do a 1.0.1 release shortly: see JuliaLang/StyledStrings.jl@dc94cfa

@kimikage
Copy link
Collaborator

kimikage commented May 6, 2024

I will merge PR #308 first to make it easier to see the CI results. (Edit: Merged)
(The problem on ARM is expected to be resolved with FixedPointNumbers v0.8.5.)
Also, I believe Dependabot will be creating a PR in a couple of days, but it is not a priority.

@tecosaur
Copy link
Contributor Author

tecosaur commented May 6, 2024

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 juialang in the repo. I'll update this PR once that's resolved.

@kimikage
Copy link
Collaborator

kimikage commented May 6, 2024

This is off-topic and outside my scope of responsibility, but what is the review process and accountability of StyledStrings.jl?
Committing without PR deprives us of the opportunity to review.
Of course, GitHub allows people to comment on commits, but that goes unnoticed by most people.

@tecosaur
Copy link
Contributor Author

tecosaur commented May 7, 2024

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 julia1-compat) can just be pushed.

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".

@tecosaur tecosaur force-pushed the master branch 2 times, most recently from 37aa8f8 to da7c643 Compare May 7, 2024 02:25
@tecosaur tecosaur marked this pull request as ready for review May 7, 2024 02:36
@tecosaur
Copy link
Contributor Author

tecosaur commented May 7, 2024

The failing tests are from:

  • RGB24/ARGB32 constructors
  • transparent gray constructors

@kimikage
Copy link
Collaborator

kimikage commented May 7, 2024

try
simplecolor = convert(StyledStrings.SimpleColor, ARGB(1, 0.6, 0, 0.4))
@test simplecolor === StyledStrings.SimpleColor(0xff, 0x99, 0x00)
catch
@test_broken convert(StyledStrings.SimpleColor, ARGB(1, 0.6, 0, 0.4))
end

Could we remove the try block?

@tecosaur
Copy link
Contributor Author

tecosaur commented May 7, 2024

I don't see why not, or why it was added in the first place?

@kimikage
Copy link
Collaborator

kimikage commented May 7, 2024

why it was added in the first place?

Because, as you know, julia1-compat didn't work before.

Copy link
Collaborator

@kimikage kimikage left a 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.)

@kimikage kimikage merged commit aa569bb into JuliaGraphics:master May 9, 2024
17 of 20 checks passed
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.

2 participants