-
Notifications
You must be signed in to change notification settings - Fork 773
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
[breaking] copy*(): allow copying broken symlinks #779
Conversation
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.
Looks good. This would be semver-major, right?
That's right @RyanZim. |
@RyanZim should we wait for more approvals or this is ready to be merged? |
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.
Do we have enough test cases where we use the actual default dereference: false
? Seems like a lot of tests have changed to true. I'm not sure if these tests with the default false are actually needed
@manidlou I was holding off merging since it's semver-major, and I wasn't sure if I wanted to release a major for just this, or bundle in some other changes. |
@RyanZim sounds good! So we can add this to the |
@JPeer264 most of those tests that are changed to |
@manidlou thanks for mentioning that. I removed the branch, as it was no longer in use. |
@manidlou Just realized this was merged with a merge-commit into |
@RyanZim go for it. |
Major version branches are a pain in general, I've decided to just commit to it, and start landing semver-major stuff on master; landed in f21048b. |
Fixes #765, fixes #638, fixes #761.