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

Added token position tests for annotations #1960

Merged
merged 6 commits into from
Dec 3, 2024

Conversation

TwoOfTwelve
Copy link
Contributor

Added some token position tests for annotations. The tests currently fail, because I decided on different lengths than the java module currently reports.

The lengths I assume for these test:

-Annotation: from @ to end of annotation name
-Annotation type begin: from the first modifier to the end of the name
All other tokens lengths are the same as in the current implementation

There are also some considerations for the module:

Most of the annotation token types seem to be unused. Maybe we should remove them?
I guess ANNO_M was supposed to be annotation members. They are currently extracted as Method tokens. Do we want to keep it like that?
Annotation arguments are currently not extracted at all. Do we want some tokens for that?

@TwoOfTwelve TwoOfTwelve marked this pull request as ready for review October 31, 2024 12:33
@tsaglam tsaglam added enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change language PR / Issue deals (partly) with new and/or existing languages for JPlag labels Oct 31, 2024
@tsaglam tsaglam requested review from a team October 31, 2024 12:35
Copy link
Contributor

@uuqjz uuqjz left a comment

Choose a reason for hiding this comment

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

Please move the comment

Copy link

sonarqubecloud bot commented Nov 4, 2024

@TwoOfTwelve TwoOfTwelve requested a review from uuqjz November 5, 2024 07:08
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

Looks good.

My initial thoughts on the open points:

Most of the annotation token types seem to be unused. Maybe we should remove them?

Yes, we can do that, probably in a separate PR to also cover #1865.

I guess ANNO_M was supposed to be annotation members. They are currently extracted as Method tokens. Do we want to keep it like that?

During the declaration of the annotation members in the annotation definition? There I would stick to method tokens.

Annotation arguments are currently not extracted at all. Do we want some tokens for that?

We could actually do this. However, ideally, it would only be for mandatory ones. If we do it for optional ones, you can affect the tokens by removing the argument or adding an argument with the same value as the default.

Let me know what you guys think!

@tsaglam
Copy link
Member

tsaglam commented Nov 21, 2024

@uuqjz ready from your side?

Copy link
Contributor

@uuqjz uuqjz left a comment

Choose a reason for hiding this comment

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

LGTM

@tsaglam tsaglam merged commit 631911f into develop Dec 3, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR that involves features, improvements and other changes language PR / Issue deals (partly) with new and/or existing languages for JPlag minor Minor issue/feature/contribution/change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants