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

address failing test on windows #93

Merged
merged 3 commits into from
Feb 7, 2025
Merged

address failing test on windows #93

merged 3 commits into from
Feb 7, 2025

Conversation

rbolgaryn
Copy link
Contributor

Fixing test for windows

@GabrielKS
Copy link
Contributor

GabrielKS commented Feb 6, 2025

What is this for/how does it work? Is it ready for review or should it be marked draft?

@jd-lara
Copy link
Member

jd-lara commented Feb 6, 2025

@rbolgaryn this is still failing

@rbolgaryn
Copy link
Contributor Author

Have not been able to figure out why it is failing on Windows. I just commented out the lines that were causing the problem in the "legacy" implementation. Since it is not used other than for testing, it should be OK. This PR should be ready to merge.

@rbolgaryn rbolgaryn changed the title try setting output type address failing test on windows Feb 6, 2025
Copy link
Contributor

@GabrielKS GabrielKS left a comment

Choose a reason for hiding this comment

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

If this is just for testing and we're sure we don't need the lines that are commented out here, might as well just delete them?

@jd-lara jd-lara self-requested a review February 7, 2025 20:06
remove unused code
Copy link
Contributor

@GabrielKS GabrielKS left a comment

Choose a reason for hiding this comment

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

Maybe add a comment about why we aren't calculating these things here. Otherwise looks fine to me.

@jd-lara jd-lara merged commit a19e646 into main Feb 7, 2025
6 checks passed
@jd-lara jd-lara deleted the rb/fix_test branch February 7, 2025 22:04
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.

3 participants