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

Ignoring contributors #612

Merged
merged 20 commits into from Jan 5, 2018

Conversation

Projects
None yet
2 participants
@micd
Contributor

micd commented Dec 28, 2017

Solution for issue #57
Added new configuration field and extended plugins to ignore contributors (or contributions done by contributors) defined in config.

Test I - release notes

  1. install new shipkit version
  2. call ./gradlew updateReleaseNotes - release notes should contain all contributors
  3. add team.ignoredContributors = ['shipkit-org'] to shipkit.gradle
  4. call ./gradlew updateReleaseNotes - release notes should not contain shipkit-org and its commits should not be included in the summary

Test II - contributors

  1. install new shipkit version
  2. call ./gradlew fetchContributors - all-contributors.json should contain all contributors
  3. add team.ignoredContributors = ['shipkit-org'] to shipkit.gradle
  4. call ./gradlew fetchContributors - all-contributors.json should not cointain shipkit-org user
}
@Override
public boolean isTrue(Contributor contributor) {

This comment has been minimized.

@wwilk

wwilk Dec 28, 2017

Contributor

Let's make this class a little more generic by accepting a string here, and explaining what format exactly this contributor string should be. This way we can reuse this class in IgnoredCommit instead of duplicating the for loop.

@wwilk

wwilk Dec 28, 2017

Contributor

Let's make this class a little more generic by accepting a string here, and explaining what format exactly this contributor string should be. This way we can reuse this class in IgnoredCommit instead of duplicating the for loop.

This comment has been minimized.

@micd

micd Jan 2, 2018

Contributor

It's better to reuse the code, but I'm not sure if IgnoredContributor should be parametrized with String. We would move some logic for ignoring contributor outside the class, because it's caller, who would have to know which part of contributor should be checked. It also closes the class from extension of ignoring rules.

I would propose extending class IgnoredContributor with method #isTrue(String contributorLogin) and provide IgnoredContributor explicitly (by exact class) to IgnoredCommit

What do you think about it?

@micd

micd Jan 2, 2018

Contributor

It's better to reuse the code, but I'm not sure if IgnoredContributor should be parametrized with String. We would move some logic for ignoring contributor outside the class, because it's caller, who would have to know which part of contributor should be checked. It also closes the class from extension of ignoring rules.

I would propose extending class IgnoredContributor with method #isTrue(String contributorLogin) and provide IgnoredContributor explicitly (by exact class) to IgnoredCommit

What do you think about it?

This comment has been minimized.

@wwilk

wwilk Jan 2, 2018

Contributor

I'm not sure if I follow.
Let me sum it up:

  • method IgnoredContributor.isTrue would be overloaded, another instance of it would take String as argument instead of Contributor object
  • IgnoredCommit uses IgnoredContributor instead of duplicating logic

Let me know if I got it right :)

We would move some logic for ignoring contributor outside the class, because it's caller, who would have to know which part of contributor should be checked.

You are right, using an object like Contributor generally makes it easier to use it - we have to only document Contributor class (what each field means exactly), and stick to this convention when we create this object and when we use it. In this case though we would have to create DefaultContributor in IgnoredCommit, which can be done but it makes it more complicated and harder to maintain (if Contributor grows, you have to remember about creating it with all necessary fields). This idea with overloading method sounds good! We can support both String (with a clear explanation what format it is in) and Contributor.

It also closes the class from extension of ignoring rules.

I don't think it does that. It's not public API, it's easy to extend a method with additional argument in a scope of the single project. If we have more of them, we can stick to using Contributor itself and pass the problem of creating this object to the caller.

@wwilk

wwilk Jan 2, 2018

Contributor

I'm not sure if I follow.
Let me sum it up:

  • method IgnoredContributor.isTrue would be overloaded, another instance of it would take String as argument instead of Contributor object
  • IgnoredCommit uses IgnoredContributor instead of duplicating logic

Let me know if I got it right :)

We would move some logic for ignoring contributor outside the class, because it's caller, who would have to know which part of contributor should be checked.

You are right, using an object like Contributor generally makes it easier to use it - we have to only document Contributor class (what each field means exactly), and stick to this convention when we create this object and when we use it. In this case though we would have to create DefaultContributor in IgnoredCommit, which can be done but it makes it more complicated and harder to maintain (if Contributor grows, you have to remember about creating it with all necessary fields). This idea with overloading method sounds good! We can support both String (with a clear explanation what format it is in) and Contributor.

It also closes the class from extension of ignoring rules.

I don't think it does that. It's not public API, it's easy to extend a method with additional argument in a scope of the single project. If we have more of them, we can stick to using Contributor itself and pass the problem of creating this object to the caller.

This comment has been minimized.

@micd

micd Jan 3, 2018

Contributor

Yes, you got it right!

If we have more of them, we can stick to using Contributor itself and pass the problem of creating this object to the caller.

I agree. I will overload method for now and if it would need extension in the future then someone will have to think about using Contributor only.

Thank you!

@micd

micd Jan 3, 2018

Contributor

Yes, you got it right!

If we have more of them, we can stick to using Contributor itself and pass the problem of creating this object to the caller.

I agree. I will overload method for now and if it would need extension in the future then someone will have to think about using Contributor only.

Thank you!

@wwilk

Thanks! Just a few small comments, please consider my feedback

@micd

This comment has been minimized.

Show comment
Hide comment
@micd

micd Jan 2, 2018

Contributor

Thank you for quick review @wwilk.
I would like to discuss one change that you have requested.

Could you address it, please?

Contributor

micd commented Jan 2, 2018

Thank you for quick review @wwilk.
I would like to discuss one change that you have requested.

Could you address it, please?

@micd

This comment has been minimized.

Show comment
Hide comment
@micd

micd Jan 3, 2018

Contributor

I have changed IgnoredContributor and definition of configuration a bit, because it turned out that login is not provided in Commit object.

Double check in IgnoredContributor#isTrue is mostly for users like 'shipkit-org', which has different commit's author name and contributor name.

Contributor

micd commented Jan 3, 2018

I have changed IgnoredContributor and definition of configuration a bit, because it turned out that login is not provided in Commit object.

Double check in IgnoredContributor#isTrue is mostly for users like 'shipkit-org', which has different commit's author name and contributor name.

@wwilk

This comment has been minimized.

Show comment
Hide comment
@wwilk

wwilk Jan 3, 2018

Contributor

Thanks! Just two more things :)

Contributor

wwilk commented Jan 3, 2018

Thanks! Just two more things :)

@wwilk

wwilk approved these changes Jan 5, 2018

Thanks! I'll create some new issues related to upgrade downstream tomorrow.

@wwilk wwilk merged commit 81ee49e into mockito:master Jan 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@micd

This comment has been minimized.

Show comment
Hide comment
@micd

micd Jan 8, 2018

Contributor

Thank you for collaboration!

Contributor

micd commented Jan 8, 2018

Thank you for collaboration!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment