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

fix: use copy(v) which works also on str, instead of v.copy() which doesn't #5417

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

corneliusroemer
Copy link

@corneliusroemer corneliusroemer commented Jul 18, 2024

resolves #5416
resolves #3404 (closed because stale, not because fixed)

Bug was possibly introduced in #3344

@msarahan @isuruf I think you are best acquainted with this part of the code base. I don't really know what's going on here - but this workaround fixes the bug for my use case.

Checklist - did you ...

Will fill out checklist if it's likely this will get merged

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

…oesn't

resolves conda#5416
resolves conda#3404 (closed because stale, not because fixed)

Bug was possibly introduced in conda#3344
@corneliusroemer corneliusroemer requested a review from a team as a code owner July 18, 2024 21:48
@conda-bot
Copy link
Contributor

We require contributors to sign our Contributor License Agreement and we don't have one on file for @corneliusroemer.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#965), and ping the bot to refresh the PR.

Copy link

codspeed-hq bot commented Jul 18, 2024

CodSpeed Performance Report

Merging #5417 will not alter performance

Comparing corneliusroemer:fix-str-copy (f1c3ce0) with main (03f99c1)

Summary

✅ 5 untouched benchmarks

@travishathaway
Copy link
Contributor

@conda-bot check

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jul 19, 2024
@travishathaway
Copy link
Contributor

@corneliusroemer,

Thanks for opening this. While we wait for the others to chime in, it would be really nice if you could add both a regression test for this and a news file entry. The regression test could go in tests/test_variants.py. We just need to test the _combined_spec_dictionaries function.

The tricky part is finding the exact data to feed into the function to recreate the stacktrace, but I hope that you might be able to generate that seeing that you can recreate this issue yourself.

Let us know if you need any help with that.

@corneliusroemer
Copy link
Author

Thanks @travishathaway! Happy to add news and try to get a test working - just wanted to check I'd not be wasting my time.

@beckermr
Copy link
Contributor

Please add a test and news, and then we can merge. This PR won't make a version of conda-build until next year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🆕 New
4 participants