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 a broken method from the signing subproject. #1679

Merged
merged 3 commits into from Apr 22, 2017

Conversation

chrisgavin
Copy link
Contributor

@chrisgavin chrisgavin commented Mar 27, 2017

Context

This method doesn't work. Note that this.signatureType is assigned signatureType rather than type (self-assignment).

Contributor Checklist

  • Review Contribution Guidelines
  • Sign Gradle CLA
  • Link to Design Spec for changes that affect more than 1 public API (that is, not in an internal package) or updates to > 20 files
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew quickCheck <impacted-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation including proper use of @since and @Incubating annotations for all public APIs
  • Recognize contributor in release notes

@bmuschko
Copy link
Contributor

Good catch. Would you mind checking if there's a test for using the method setSignatureType and add one if it doesn't exist yet?

@chrisgavin chrisgavin changed the title Remove a broken method from the signing subproject. Fix a broken method from the signing subproject. Mar 31, 2017
@chrisgavin
Copy link
Contributor Author

I've realized that the method is still needed for use in DSL so I've fixed it and added a test as requested.

@w25r w25r added this to the 4.0 RC1 milestone Apr 17, 2017
@w25r
Copy link

w25r commented Apr 17, 2017

Hi @chrisgavin,

Why don't you think we should remove the method? I'm not sure what you mean by still needed for use in DSL Could you please elaborate?

@chrisgavin
Copy link
Contributor Author

As I mentioned in my comment, I don't think the method should be removed: the Gradle DSL looks for a method with the name signatureType or Gradle files relying on it will not work. I most certainly think it should be fixed though so that the method works as expected though.

@w25r
Copy link

w25r commented Apr 17, 2017

All the following calls in a .gradle file

signatureType = newType
signatureType(newType)
setSignatureType(newType)

delegate to the setSignatureType method, which works as expected. I think only explicit calls from Java plugins may reach the flawed method. As such, I'm tempted to remove it rather than fix it.

@chrisgavin
Copy link
Contributor Author

Ah yes, I see what you mean. I have removed the method.

@w25r w25r merged commit 7aa37be into gradle:master Apr 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants