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

feat(ast): extend support for annotation named parameters #1012

Merged
merged 10 commits into from
Sep 6, 2022

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented Jul 7, 2022

[updated]
This change is to extend annotation arguments support.
Before: Only one argument that consist of a single String can be added to annotation description.
After: For single unnamed parameter/description, add support for ValueExpr and VariableExpr in addition to String. Also allow adding multiple named parameters of type VariableExpr. So that annotations like below can be generated:

@ConditionalOnClass(VisionServiceClient.class)
@ConditionalOnProperty(value = "spring.cloud.gcp.vision.enabled", matchIfMissing = true)

this improvement is an addition and does not affect existing usage of setDescription(String description).
Added tests in JavaWriterVisitorTest.java also shows the new feature usages.

@zhumin8 zhumin8 marked this pull request as ready for review July 8, 2022 15:34
@zhumin8 zhumin8 requested review from a team as code owners July 8, 2022 15:34
@zhumin8 zhumin8 requested a review from blakeli0 July 8, 2022 15:34
import com.google.auto.value.AutoValue;

@AutoValue
public abstract class ClazzValue implements ObjectValue {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a way to achieve the same thing, see the code here and the golden test file here. I think it's better not to have different ways to achieve the same thing, it would create more maintenance burden in the future. If you want, you could extract the common logic to a util method/class, something like createClazzValueExpr or createClazzVariableExpr, which only takes type as an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the tip. I couldn't wrap my head around needing a Value instead of Variable.

Arrays.asList(ValueExpr.withValue(StringObjectValue.withValue(description))));
}

public Builder setDescriptions(List<Expr> exprList) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this improvement in general, but we should be careful exposing a setter that can set descriptions to any Expr, you can technically pass a NewObjectExpr or a ReturnExpr and everything will work until we try to compile the generated code, which could be too late to find an issue.
Correct me if I'm wrong, I think we only allow ValueExpr, AssignmentExpr, or VariableExpr(for clazz if you remove CLazzValue). So we could either add some type checks in the build() method, or only expose methods to add descriptions for the above types, like addDescription(ValueExpr expr), addDescription(AssignmentExpr expr) and addDescription(VariableExpr expr).
Basically anything that could restrict people from passing any types of Expr would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, let me refactor this part and expose methods for each expr type.

@sonarcloud
Copy link

sonarcloud bot commented Jul 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

97.5% 97.5% Coverage
0.0% 0.0% Duplication

@zhumin8 zhumin8 requested a review from blakeli0 July 15, 2022 17:25
@zhumin8
Copy link
Contributor Author

zhumin8 commented Aug 24, 2022

Adding a small enhancement to this PR since it's related.
In addition to previous changes, I added an optional field to VariableExpr allowing Annotations.
Prior to this, only annotations on ClassDefinition and MethodDefinition are supported. With this addition, I will be able to generate code like:

  @NestedConfigurationProperty
  private final Credentials credentials = new Credentials();

  // or 
  @Autowired
  private LanguageServiceClient autoClient;

Field annotations are pretty common in Spring syntax, this enhancement is needed to generate Spring autoconfig code as I planned.
Note: Added a todo note in VariableExpr only as a nice to have feature for future, it's not blocking any use cases for now. But it seems reasonable to have target info in AnnotationNode and apply checks on it when adding annotation nodes.

@sonarcloud
Copy link

sonarcloud bot commented Aug 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

98.0% 98.0% Coverage
0.0% 0.0% Duplication

@zhumin8 zhumin8 merged commit 5d3ff75 into main Sep 6, 2022
@zhumin8 zhumin8 deleted the annotation-node-args branch September 6, 2022 13:46
emmileaf added a commit that referenced this pull request Nov 3, 2022
* This PR is a follow-up patch to #1012 which added support for annotations on VariableExpr. It updates ImportWriterVisitor so that import generation also covers these annotations.
emmileaf added a commit that referenced this pull request Nov 3, 2022
* This PR is a follow-up patch to #1012 which added support for annotations on VariableExpr. It updates ImportWriterVisitor so that import generation also covers these annotations.
suztomo pushed a commit that referenced this pull request Dec 16, 2022
This change is to extend annotation arguments support.
**Before**: Only one argument that consist of a single `String` can be added to annotation description.
**After**:  For single unnamed parameter/description, add support for `ValueExpr` and `VariableExpr` in addition to `String`. Also allow adding multiple named parameters of type `VariableExpr`. So that annotations like below can be generated:


```
@ConditionalOnClass(VisionServiceClient.class)
@ConditionalOnProperty(value = "spring.cloud.gcp.vision.enabled", matchIfMissing = true)
```
this improvement is an addition and does not affect existing usage of `setDescription(String description)`.
Added tests in [JavaWriterVisitorTest.java](https://github.com/googleapis/gapic-generator-java/pull/1012/files#diff-60163f71e845b4cc97f9ccd7101646aa243fa5317a77cd326a7f61143618f62f) also shows the new feature usages.

Adding a small enhancement to this PR since it's related.
In addition to previous changes, I added an optional field to VariableExpr allowing Annotations. 
Prior to this, only annotations on ClassDefinition and MethodDefinition are supported. With this addition, I will be able to generate code like:
```
  @NestedConfigurationProperty
  private final Credentials credentials = new Credentials();

  // or 
  @Autowired
  private LanguageServiceClient autoClient;
```
Field annotations are pretty common in Spring syntax, this enhancement is needed to generate Spring autoconfig code as I planned. 
Note: Added a todo note in `VariableExpr` only as a nice to have feature for future, it's not blocking any use cases for now.   But it seems reasonable to have target info in `AnnotationNode` and apply checks on it when adding annotation nodes.
suztomo pushed a commit that referenced this pull request Dec 16, 2022
* This PR is a follow-up patch to #1012 which added support for annotations on VariableExpr. It updates ImportWriterVisitor so that import generation also covers these annotations.
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 26, 2023
)

This change is cherry-picked from an AST fix made in the spring branch (https://github.com/googleapis/gapic-generator-java/pull/1208). It fixes `ImportWriter` to account for types introduced by annotation parameters, which as enabled in #1012.
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.

2 participants