Add Project.propertyOrDefault method #578

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@szpak
Contributor

szpak commented Feb 28, 2016

TL;TR; A convenient way to set a default value for a property which could be not found. Without that hasProperty has to be used as a guard check.

Longer rationale.

Many plugins require certain properties to be set and fail otherwise in a configuration phase, even if no task from given plugin is called. This often refers to credentials (usernames, passwords, API keys, ...) which are set in ~/.gradle/gradle.properties. It is required to use a guard check with project.hasProperty:

codeCoverageOnline {
  key = project.hasProperty('ccoKey') ? project.getProperty('ccoKey') : 'N/A'

or create an empty value in project specific gradle.properties. With Project.propertyOrDefault there is a convenient way to a fallback value.

codeCoverageOnline {
  key = project.propertyOrDefault('ccoKey', 'N/A')

Btw, the tests I created are probably in not the best possible place, but I wasn't able to find a test which verifies reading properties itself.

@pioterj pioterj added the cla:yes label Feb 29, 2016

@pioterj pioterj added this to the waiting-for-gradle-team milestone Feb 29, 2016

@mark-vieira

This comment has been minimized.

Show comment
Hide comment
@mark-vieira

mark-vieira Mar 3, 2016

Member

We could potentially also introduce a new method that simply returns null rather than throwing an exception when a property cannot be found. We have this pattern in many of our 'container' classes where the getBy and findBy methods fail or return null, respectively when a matching item isn't found. Adding a findProperty() method would allow users to do something like:

def foo = findProperty('unknownProperty') ?: 'default'

Another option is to simply to provide this behavior in an overloaded property() method. I find propertyOrDefault is somewhat verbose for a method whose sole purpose is to provide a concise solution to a common use case. Thoughts?

Member

mark-vieira commented Mar 3, 2016

We could potentially also introduce a new method that simply returns null rather than throwing an exception when a property cannot be found. We have this pattern in many of our 'container' classes where the getBy and findBy methods fail or return null, respectively when a matching item isn't found. Adding a findProperty() method would allow users to do something like:

def foo = findProperty('unknownProperty') ?: 'default'

Another option is to simply to provide this behavior in an overloaded property() method. I find propertyOrDefault is somewhat verbose for a method whose sole purpose is to provide a concise solution to a common use case. Thoughts?

@pioterj pioterj assigned breskeby and unassigned melix Mar 3, 2016

@oehme

This comment has been minimized.

Show comment
Hide comment
@oehme

oehme Mar 3, 2016

Member

👍 for findProperty(), as it is consistent with other parts of the code base.

Member

oehme commented Mar 3, 2016

👍 for findProperty(), as it is consistent with other parts of the code base.

@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Mar 3, 2016

Member

I'm also in favor for findProperty

Member

breskeby commented Mar 3, 2016

I'm also in favor for findProperty

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Mar 3, 2016

Contributor

I also find findProperty consistent. A small inconvenience could be that property set to null will be indistinguishable from missing property. Nevertheless, as -Pfoo set foo property to an empty string it shouldn't be a problem.

I will rework the code. Can you propose any better place to put tests for that feature (than CommandLineIntegrationTest)?

Contributor

szpak commented Mar 3, 2016

I also find findProperty consistent. A small inconvenience could be that property set to null will be indistinguishable from missing property. Nevertheless, as -Pfoo set foo property to an empty string it shouldn't be a problem.

I will rework the code. Can you propose any better place to put tests for that feature (than CommandLineIntegrationTest)?

@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Mar 3, 2016

Member

great. I think DynamicObjectIntegrationTest is the best place to put the according integration tests.

Member

breskeby commented Mar 3, 2016

great. I think DynamicObjectIntegrationTest is the best place to put the according integration tests.

+ public Object propertyOrDefault(String propertyName, Object defaultValue) {
+ try {
+ return property(propertyName);
+ } catch (MissingPropertyException ignored) {

This comment has been minimized.

@mark-vieira

mark-vieira Mar 11, 2016

Member

We should be delegating to hasProperty() here to determine if the property exists rather than relying on exceptions for control flow.

@mark-vieira

mark-vieira Mar 11, 2016

Member

We should be delegating to hasProperty() here to determine if the property exists rather than relying on exceptions for control flow.

This comment has been minimized.

@szpak

szpak Mar 14, 2016

Contributor

Good catch

@szpak

szpak Mar 14, 2016

Contributor

Good catch

[#578] Add Project.findProperty method
A convenient to deal with properties which could be not set.
Without that hasProperty has to be used as a guard check.
@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Mar 14, 2016

Contributor

I updated my PR.

Btw, Idea with default formatting seems to do not like alone while spaces at the beginning of a line and automatically was reformatting DynamicObjectIntegrationTest. I could override it with out-of-IDE changes, but the issue would probably be back at the next modification in that file.

Contributor

szpak commented Mar 14, 2016

I updated my PR.

Btw, Idea with default formatting seems to do not like alone while spaces at the beginning of a line and automatically was reformatting DynamicObjectIntegrationTest. I could override it with out-of-IDE changes, but the issue would probably be back at the next modification in that file.

breskeby added a commit that referenced this pull request Mar 15, 2016

[#578] Add Project.findProperty method
A convenient to deal with properties which could be not set.
Without that hasProperty has to be used as a guard check.
@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Mar 15, 2016

Member

Thanks again Marcin for this pull request. It was merged manually and will be part of the Gradle 2.13 release. A small change that will make a difference for a lot of gradle users.
cheers,
René

Member

breskeby commented Mar 15, 2016

Thanks again Marcin for this pull request. It was merged manually and will be part of the Gradle 2.13 release. A small change that will make a difference for a lot of gradle users.
cheers,
René

@breskeby breskeby closed this Mar 15, 2016

@szpak szpak deleted the szpak:propertyOrDefault branch Mar 15, 2016

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Apr 6, 2016

Contributor

Guys, I've seen the release notes for 2.13-rc-1. This change is small, yet useful, but without mentioning it in the release notes (in the "External contributions" section it is almost not visible) it will remain rarely used. Maybe it would be good to add it somewhere to the release notes (maybe something like the "Fixed issues" section with "Small(er) enhancements")?

Contributor

szpak commented Apr 6, 2016

Guys, I've seen the release notes for 2.13-rc-1. This change is small, yet useful, but without mentioning it in the release notes (in the "External contributions" section it is almost not visible) it will remain rarely used. Maybe it would be good to add it somewhere to the release notes (maybe something like the "Fixed issues" section with "Small(er) enhancements")?

@breskeby

This comment has been minimized.

Show comment
Hide comment
@breskeby

breskeby Apr 6, 2016

Member

Hey Marcin, I've added an issue (https://issues.gradle.org/browse/GRADLE-3430) linked to this work. This makes it now also visible as fixed issue in the release notes. https://docs.gradle.org/release-candidate/release-notes#fixed-issues

cheers,
René

Member

breskeby commented Apr 6, 2016

Hey Marcin, I've added an issue (https://issues.gradle.org/browse/GRADLE-3430) linked to this work. This makes it now also visible as fixed issue in the release notes. https://docs.gradle.org/release-candidate/release-notes#fixed-issues

cheers,
René

@eljobe

This comment has been minimized.

Show comment
Hide comment
@eljobe

eljobe Apr 6, 2016

I also just added it to the New and Noteworthy section of the Release notes.
6201f46

eljobe commented Apr 6, 2016

I also just added it to the New and Noteworthy section of the Release notes.
6201f46

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Apr 6, 2016

Contributor

Thanks guys!

Contributor

szpak commented Apr 6, 2016

Thanks guys!

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