You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2022/02/16 17:29:44 UTC
[GitHub] [groovy] eric-milles opened a new pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
eric-milles opened a new pull request #1687:
URL: https://github.com/apache/groovy/pull/1687
For default values, I want to use `map.containsKey(name) ? map.name : defaultValue` instead of `map.name ?: defaultValue` -- in case `null` or `0` or another falsy value is supplied. This change does not give the map a chance to supply a default value, but I thought that was okay in order to allow user to supply falsy value explicitly.
One case that was hard to understand comes form the `record` tests. The example has `null` passed for an `int` and results in `0` whereas this code produces a cast exception. I wanted to understand if that was desired or just a happy accident.
```groovy
assert new ColoredPoint(x: 0, y: null).toString() == 'ColoredPoint[x=0, y=0, color=white]'
```
A null test could be inserted for primitives, but it would be more complex and would probably mean one of the operands was evaluated twice. Probably not a huge concern, but I thought I would meantion it.
https://issues.apache.org/jira/browse/GROOVY-10497
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert merged pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
Posted by GitBox <gi...@apache.org>.
paulk-asert merged pull request #1687:
URL: https://github.com/apache/groovy/pull/1687
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1687:
URL: https://github.com/apache/groovy/pull/1687#issuecomment-1044455780
I am in favor of removing the truthy and null-handling aspect. I think the intention was just to handle the null case but I think we shouldn't even handle that - instead some judicial `IllegalArgumentException`s might be in order - whenever null is provided for a primitive or for the namedArgs map. We could always add a `nullSafePrimitive` annotation attribute to `NamedVariant` and/or `MapConstructor` which could instead provide some default instead of an `IllegalArgumentException`, but I suggest we don't do that for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1687:
URL: https://github.com/apache/groovy/pull/1687#issuecomment-1045245638
Yes. And I was suggesting:
- we can add in some additional throw IllegalArgumentException statements in places where the resulting error message is confusing, e.g. when we call containsKey on a null map
- potentially add in an option to keep the replace null with default (basically keep lines 283-285 or similar as an option) at a later date if there is demand, we are kind of breaking behavior which some might be relying on - having the option would be a workaround for them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1687:
URL: https://github.com/apache/groovy/pull/1687#issuecomment-1044455780
I am in favor of removing the truthy and null-handling aspect. I think the intention was just to handle the null case but I think we shouldn't even handle that. We could always add a `nullSafePrimitive` flag to `NamedVariant` and/or `MapConstructor` but I suggest we don't do that for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1687:
URL: https://github.com/apache/groovy/pull/1687#issuecomment-1045415536
I merged and will adjust a little as per my comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1687:
URL: https://github.com/apache/groovy/pull/1687#issuecomment-1044796879
Not sure I follow fully. Are you saying go with the replacement of elvisX with ternaryX/containsKey, but leave off the primitive null check (lines 283-285)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1687:
URL: https://github.com/apache/groovy/pull/1687#issuecomment-1044455780
I am in favor of removing the truthy and null-handling aspect. I think the intention was just to handle the null case but I think we shouldn't even handle that - instead some judicial `IllegalArgumentException`s might be in order - whenever null is provided for a primitive or for the namedArgs map. We could always add a `nullSafePrimitive` flag to `NamedVariant` and/or `MapConstructor` which could instead provide some default instead of an `IllegalArgumentException`, but I suggest we don't do that for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on pull request #1687: GROOVY-10497: `@NamedVariant`: don't replace explicit value with default
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1687:
URL: https://github.com/apache/groovy/pull/1687#issuecomment-1041912453
`map.getOrDefault(name,defaultValue)` is another option. The ternary avoids evaluating `defaultValue` if it is not used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org