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

Added support of the steiger config for windows #134

Merged

Conversation

mx-ryabov
Copy link
Contributor

Hey!
My version of tests looks a bit overwhelming, I also thought about a simple function that would take a platform parameter and depending on the value would prepend C:, but I'm not sure if that's the right way to do tests. However, if you think otherwise, I can rework it (or am open to any other suggestions)

Copy link

changeset-bot bot commented Nov 27, 2024

🦋 Changeset detected

Latest commit: 8395a9b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
steiger Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Thanks for making a PR! Sorry it took a while to get a review. The solution looks solid. The tests are a bit hard to parse, I think we can make them simpler by simply writing two its and then skipping one or the other based on the platform. Our CI runs in both OSes, so we will cover everything that way

For reference: https://vitest.dev/api/#test-runif

.changeset/early-stingrays-appear.md Outdated Show resolved Hide resolved
.changeset/early-stingrays-appear.md Outdated Show resolved Hide resolved
@mx-ryabov
Copy link
Contributor Author

I considered a similar to yours approach but I thought about the case when somebody on a unix machine could break something for windows and this developer won't find out it until the CI checks passed.
But anyway all your comments are resolved, let me know if you need something else. Thank you!

@mx-ryabov mx-ryabov requested a review from illright December 1, 2024 18:38
Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments! Nothing else on my side, great job, let's merge.

As for your concern about devs not finding out until CI — there's a lot of places where that can happen, and that's okay, that's why we have CI on several OSes, to alert devs that something might not work on someone else's machine

@illright illright merged commit 114d59c into feature-sliced:master Dec 2, 2024
7 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