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

Changed methodname to: PROPERTY_EDITOR.canNotUsePropertyEditors() #80

Merged
merged 4 commits into from
Jun 10, 2014
Merged

Changed methodname to: PROPERTY_EDITOR.canNotUsePropertyEditors() #80

merged 4 commits into from
Jun 10, 2014

Conversation

drapostolos
Copy link
Contributor

I found the method name confusing, i.e. return null when we "can use property editors".

if (canUsePropertyEditors())
           return null;

So I figured "canNotUsePropertyEditors" would be a less confusing name.

/Alex

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 80a18a5 on drapostolos:master into f461fa9 on lviggiano:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 80a18a5 on drapostolos:master into f461fa9 on lviggiano:master.

@drapostolos
Copy link
Contributor Author

I agree, a positive form is easier to read/understand.

Should this be changed too?:

``` java`
! isPropertyEditorDisabled()


So it becomes:

``` java
private boolean canUsePropertyEditors() {
     return isPropertyEditoryAvailable() && isPropertyEditorEnabled();
}

@lviggiano
Copy link
Collaborator

I would keep the method name as isPropertyEditorDisabled(), since PropertyEditor are enabled by default and to change that, you need to specify a system property as 'org.aeonbits.owner.property.editor.disabled=true'

        private boolean isPropertyEditorDisabled() {
            return Boolean.getBoolean("org.aeonbits.owner.property.editor.disabled");
        }

I added this because on Android the PropertyEditor classes are not available, so OWNER will check if these are available in the classpath and disable it automatically. The system property instead allows this to be done by the user. It came handy for unit testing; I think it's undocumented, I should document it possibly.

@lviggiano
Copy link
Collaborator

I just did a minor refactoring on Converters class, ensure to pull it from my master branch before to apply your changes. Thanks.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling b1ec204 on drapostolos:master into f461fa9 on lviggiano:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling b1ec204 on drapostolos:master into f461fa9 on lviggiano:master.

lviggiano pushed a commit that referenced this pull request Jun 10, 2014
Changed methodname to: rectored PROPERTY_EDITOR converter (method name was wrong)
@lviggiano lviggiano merged commit ddfdfc0 into matteobaccan:master Jun 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants