-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix(apca-silver-plus): don't reverse in place #132
Conversation
🦋 Changeset detectedLatest commit: f7c60d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@giamir , just tagging you here for visibility, cause I know GitHub notifications can be lost easily 🙂 |
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.
Thanks @MatteoPieroni for the PR and sorry for the late feedback on it.
I think I understand the problem you have encountered but before merging the PR I would like to have a corresponding test in place to prevent similar regressions in the future.
I think you should be able to add a test case in the index.test.ts file under the silver plus describe showcasing the issue this PR is fixing.
Let me know if you need further guidance and thanks for the contribution.
Thank you @giamir! I've added the test, not sure on the test title, happy to change if you have a better idea 🙂 |
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.
We are good to merge. Thanks for the contribution @MatteoPieroni and for your patience. Will cut a new patch release shortly.
Hey folks, I found a small issue with the
silver
check:we were reversing the array in place, which meant the array was being reversed and re-reversed. I switched it to reverse a copy of the original array, so that we don't taint it.