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/15 23:55:05 UTC
[GitHub] [groovy] eric-milles opened a new pull request #1686: GROOVY-9158, GROOVY-10176, GROOVY-10484: `NamedParam` checking enhancement
eric-milles opened a new pull request #1686:
URL: https://github.com/apache/groovy/pull/1686
Wrapping `NamedParam` annotations in a `NamedParams` container is straightforward. For default values, I wanted to use `map.containsKey(name) ? map.name : defaultValue` instead of `map.name ? 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.
@paulk-asert 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]'
```
https://issues.apache.org/jira/browse/GROOVY-10484
https://issues.apache.org/jira/browse/GROOVY-10176
https://issues.apache.org/jira/browse/GROOVY-9158
--
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 #1686: GROOVY-10484: `@NamedVariant`: bind `@NamedParam` into `@NamedParams`
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1044421069
I still wonder whether forcing the collation early should be a 2.5 only thing - simply because if other frameworks like Grails or Micronaut did similar NamedParam additions we'd be requiring them to also collate. Let's commit as is for now. If it ever becomes a problem we can always refactor on 3 and above.
--
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 #1686: GROOVY-10484: `@NamedVariant`: bind `@NamedParam` into `@NamedParams`
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1044799201
I can look into STCV changes instead if there is concern that others may be adding `@NamedParam` without the wrapper. Are there any examples of this?
--
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 #1686: GROOVY-9158, GROOVY-10176, GROOVY-10484: `NamedParam` checking enhancement
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1041701400
The change for GROOVY-10484 could be separated out. The other changes are to address 10176 and 9158 (which can be argued have been addressed already by changes in Groovy 4) and:
Consider the following:
```groovy
class C {
C(int i, int j = 42) {
print "$i $j"
}
}
new C(i:0,j:0) // prints "0 42"
```
I could file this as a separate issue and move the changes to a new branch.
--
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 edited a comment on pull request #1686: GROOVY-9158, GROOVY-10176, GROOVY-10484: `NamedParam` checking enhancement
Posted by GitBox <gi...@apache.org>.
eric-milles edited a comment on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1041701400
The change for GROOVY-10484 could be separated out. The other changes are to address 10176 and 9158 (which can be argued have been addressed already by changes in Groovy 4) and:
```groovy
class C {
C(int i, int j = 42) {
print "$i $j"
}
}
new C(i:0,j:0) // prints "0 42"
```
I could file this as a separate issue and move the changes to a new branch.
Also there is differing handling for implicit vs. explicit `@NamedParam` annotation:
```java
Expression arg = propX(varX(map), name);
// implicit annotation:
Expression fallback = defaultValue == null && isPrimitiveType(type) ? defaultValueX(type) : defaultValue;
Expression argOrDefault = defaultValue == null && !isPrimitiveType(type) ? arg : elvisX(arg, fallback);
// explicit annotation:
Expression argOrDefault = defaultValue != null ? elvisX(arg, defaultValue) : arg;
return asType(argOrDefault, type, coerce);
```
--
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 edited a comment on pull request #1686: GROOVY-9158, GROOVY-10176, GROOVY-10484: `NamedParam` checking enhancement
Posted by GitBox <gi...@apache.org>.
eric-milles edited a comment on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1041701400
The change for GROOVY-10484 could be separated out. The other changes are to address 10176 and 9158 (which can be argued have been addressed already by changes in Groovy 4) and:
```groovy
class C {
C(int i, int j = 42) {
print "$i $j"
}
}
new C(i:0,j:0) // prints "0 42"
```
I could file this as a separate issue and move the changes to a new branch.
Also there is differing handling for implicit vs. explicit `@NamedParam` annotation:
```java
Expression arg = propX(varX(map), name);
// implicit annotation:
Expression fallback = defaultValue == null && isPrimitiveType(type) ? defaultValueX(type) : defaultValue;
Expression argOrDefault = defaultValue == null && !isPrimitiveType(type) ? arg : ternaryX(arg, arg, fallback);
// explicit annotation:
Expression argOrDefault = defaultValue != null ? elvisX(arg, defaultValue) : arg;
return asType(argOrDefault, type, coerce);
```
--
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 #1686: GROOVY-9158, GROOVY-10176, GROOVY-10484: `NamedParam` checking enhancement
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1041493348
I am still looking at the changes. Some effort was put into making primitives behave in an "expected" way but whether we are consistent to what happens elsewhere needs further investigation. I am wondering whether it would be better to adapt `StaticTypeCheckingVisitor` to know about the individual `NamedParam` annotations not just the collector. The annotation collector is already currently applied but not until classgen phase.
--
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 #1686: GROOVY-9158, GROOVY-10176, GROOVY-10484: `NamedParam` checking enhancement
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1041892273
This commit has been reduced to 10484 changes only.
--
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 #1686: GROOVY-10484: `@NamedVariant`: bind `@NamedParam` into `@NamedParams`
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1053938214
@eric-milles The alternative would look something like: #1693
--
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 #1686: GROOVY-10484: `@NamedVariant`: bind `@NamedParam` into `@NamedParams`
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1042096674
NOTE: This is a solution that I can backport to Groovy 2.5, which lacks the `@Repeatable` annotation: https://github.com/apache/groovy/blob/GROOVY_2_5_X/src/main/groovy/groovy/transform/NamedParam.java
--
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 closed pull request #1686: GROOVY-10484: `@NamedVariant`: bind `@NamedParam` into `@NamedParams`
Posted by GitBox <gi...@apache.org>.
paulk-asert closed pull request #1686:
URL: https://github.com/apache/groovy/pull/1686
--
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 edited a comment on pull request #1686: GROOVY-9158, GROOVY-10176, GROOVY-10484: `NamedParam` checking enhancement
Posted by GitBox <gi...@apache.org>.
eric-milles edited a comment on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1041701400
The change for GROOVY-10484 could be separated out. The other changes are to address 10176 and 9158 (which can be argued have been addressed already by changes in Groovy 4) and:
```groovy
class C {
C(int i, int j = 42) {
print "$i $j"
}
}
new C(i:0,j:0) // prints "0 42"
```
I could file this as a separate issue and move the changes to a new branch.
--
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 #1686: GROOVY-10484: `@NamedVariant`: bind `@NamedParam` into `@NamedParams`
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1055403230
PR#1693 was merged in Groovy 3 and above. This PR was merged with minor adjustments onto 2_5_X.
--
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 #1686: GROOVY-10484: `@NamedVariant`: bind `@NamedParam` into `@NamedParams`
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1686:
URL: https://github.com/apache/groovy/pull/1686#issuecomment-1053938214
@eric-milles The alternative would look something like: #1693. I say alternative, but both could be applied. My preference would be to go with #1693 for 3 and above and merge this PR as well in 2.5 if needed. Do you see any problem with that?
--
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