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

chore: Replace ctor with std::sync::Once #115

Closed
wants to merge 1 commit into from

Conversation

Marwes
Copy link

@Marwes Marwes commented Apr 26, 2023

A std::sync::Once should effectively do the same thing as ctor, only difference is an extra check before failures whether the initialization has already been done.

Haven't tested this (on windows) but it ought to be fine. If nothing else maybe someone on windows can use this PR to test or just have faith that it is ok :)

A `std::sync::Once` should effectively do the same thing as ctor, only difference is an extra check before failures whether the initialization has already been done.
Copy link
Collaborator

@tommilligan tommilligan left a comment

Choose a reason for hiding this comment

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

This looks amazing, thank you! I agree that the Once pattern should do the same job and is far less magic than ctor, given that everyone knows about once_cell and I had to look up what ctor does.

As this is a Windows specific feature, I agree there needs to be some Windows QA before this is changed. Ideally I'd like to setup some representative CI that actually verifies this feature, but I'd settle for this being tried out on a Windows machine.

I'd actually be interested to see if this has any compilation speed impact on non-windows platforms, not that I think it should.

@sjackman
Copy link

@tommilligan Do you have a moment to review this PR?

@tommilligan
Copy link
Collaborator

@sjackman the code looks good to me - from my view this is blocked on someone testing on Windows system and checking it still works.

If anyone can verify this branch and post a screenshot of the example running in powershell or something similar, that's good enough for me.

@tommilligan tommilligan mentioned this pull request Jul 6, 2023
@dbanty
Copy link

dbanty commented Jul 6, 2023

Anyone have suggestion on how to test this? I thought, from the description of output_vt100, that I would not see any colors in PowerShell or Command Prompt if init was not called. However, I ran cargo run --example pretty_assertion in both PowerShell and Command Prompt and I get pretty colors even with output_vt100 completely removed 😅.

This is on Windows 10, is this only required for older versions of Windows?

@tommilligan
Copy link
Collaborator

@dbanty Thank you for testing this.

After a quick dive into the git history, it appears this dependency was added by the dependency's author (Phundrak) in a commit back in 2019. The dependency itself is very low star count, and the download count is almost all due to it's inclusion in pretty_assertions.

Therefore, I'm going to conclude it's probably no longer necessary/not doing anything useful, and remove it in the next minor version. If anyone screams, we can reinstate it after getting details of the failure environment for testing.

Thanks all for your contributions to investigating this!

@tommilligan
Copy link
Collaborator

Closing in favour of #118

@tommilligan tommilligan closed this Jul 6, 2023
@tommilligan
Copy link
Collaborator

For reference, the original PR/discussion of why this was initially added: #28

The original states that ANSI codes where not supported by default in powershell, which appears to no longer be the case.

@tommilligan
Copy link
Collaborator

ctor has been removed in v1.4.0

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.

None yet

4 participants