Skip to content
This repository was archived by the owner on Aug 19, 2025. It is now read-only.

Conversation

@k1dav
Copy link
Contributor

@k1dav k1dav commented May 25, 2020

No description provided.

@hramezani
Copy link
Contributor

Hi @k1dave6412 , Could you please add a test for this patch?

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented May 26, 2020

Actually I’m pretty okay with changes like this one not adding extra test cases, since they end up being:

Code: “Call this function with this new argument”
Test: “Test that the code called the function with this new argument”

Which doesn’t actually tend to add a lot of value.

@k1dav
Copy link
Contributor Author

k1dav commented May 26, 2020

@hramezani I added the simple test

@hramezani
Copy link
Contributor

Actually I’m pretty okay with changes like this one not adding extra test cases, since they end up being:

Code: “Call this function with this new argument”
Test: “Test that the code called the function with this new argument”

Which doesn’t actually tend to add a lot of value.

@tomchristie Thanks for the explanation. I agree with you.

@k1dave6412 Sorry, we don't need the test. Please remove it.

@k1dav
Copy link
Contributor Author

k1dav commented May 26, 2020

@hramezani Ok, I remove it

Copy link
Contributor

@hramezani hramezani left a comment

Choose a reason for hiding this comment

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

@k1dave6412 Thanks 👍

@k1dav
Copy link
Contributor Author

k1dav commented May 27, 2020

@hramezani When will the PR be merged?

@hramezani
Copy link
Contributor

@k1dave6412 I am not a merger of this project. let's wait for @tomchristie to merge it. I believe he will do that when he has time.

@lbatteau
Copy link

@tomchristie can you please review and merge this PR, it's a very simple fix and a huge blocker right now. 🙏

@lbatteau
Copy link

@Kludex
Copy link
Contributor

Kludex commented Aug 20, 2022

I don't know how to checkout on fork's master branch locally, and I needed to apply black here... So I've created #79 - co-author trailer is added there.

@Kludex Kludex closed this Aug 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants