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

fix: remove annotation from the pretty-printed string if it is deleted from the model #4282

Closed
wants to merge 2 commits into from

Conversation

algomaster99
Copy link
Contributor

@algomaster99 algomaster99 commented Nov 12, 2021

Fixes #4218

I am still looking for a fix for this, but I think the problem could be with the changeCollector. The size of the map elementToChangeRole is 0, which is unusual as we have altered the type by deleting the annotation attached to it. Thus, I am concluding that it fails to listen for deletion of annotation in the model build for the resource file in this PR.

EDIT:

onChange is not being called when I delete an annotation, however, it is called when I delete an instance of CtField inside a CtType. This should confirm that what I am saying above is right.

@algomaster99

This comment has been minimized.

@monperrus
Copy link
Collaborator

onChange is not being called when I delete an annotation, however, it is called when I delete an instance of CtField inside a CtType. This should confirm that what I am saying above is right.

seems you have the core idea of the fix :)

@algomaster99
Copy link
Contributor Author

algomaster99 commented Nov 12, 2021

@monperrus, and here comes this fix!

When all annotations are deleted, this.annotations is set to emptyList, but we also need to register this change in the environment. The patch does exactly that.

Note that I also removed the the listener just after this.annotations.clear because we shall be registering addition in the loop which follows.

@algomaster99 algomaster99 marked this pull request as ready for review November 12, 2021 10:54
@algomaster99
Copy link
Contributor Author

@monperrus @slarse @nharrand need your reviews. The tests have passed.

@monperrus
Copy link
Collaborator

nice, LGTM. thanks @algomaster99

there are many more test cases in #4018, does this fix make them all pass?

@algomaster99
Copy link
Contributor Author

@monperrus I think I linked the wrong issue. 🤦‍♂️ Let me find the correct one.

@algomaster99
Copy link
Contributor Author

algomaster99 commented Nov 12, 2021

@monperrus I found it. It was #4218 instead of 4018.

Let me reword the first commit message before we merge. I am doing that just for the logs even though it won't matter because we shall squash and merge.

@algomaster99
Copy link
Contributor Author

@monperrus Some certifcate has expired which is making tests fail.

Run actions/setup-java@8db439b6b47e5e12312bf036760bbaa6893481ac
  with:
    java-version: 16
    distribution: temurin
    java-package: jdk
    architecture: x64
    check-latest: false
    server-id: github
    server-username: GITHUB_ACTOR
    server-password: GITHUB_TOKEN
    overwrite-settings: true
    job-status: success
  env:
    JAVA_DISTRIBUTION: temurin
    MAVEN_OPTS: -Djava.src.version=16 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
Trying to resolve the latest version from remote
Error: certificate has expired

Do you know what certificate are they talking about? Failing step.

@algomaster99 algomaster99 deleted the delete-annotation branch November 12, 2021 13:02
@algomaster99 algomaster99 restored the delete-annotation branch November 12, 2021 13:02
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.

SniperPrettyPrinter - Annotation not getting deleted on transformation
2 participants