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

Project Wide Static Analysis Annotations #235

Merged
merged 16 commits into from
Jun 4, 2022

Conversation

Sxtanna
Copy link
Contributor

@Sxtanna Sxtanna commented Oct 16, 2021

This PR adds JetBrains annotations throughout the entire project, including:

  • @NotNull
  • @Nullable
  • @Contract

This addresses a concern originally raised by me here:
#133 (comment)

And later similarly referenced here:
#196

( WIP please don't flame me ❤️ )

@ljacqu
Copy link
Member

ljacqu commented Oct 17, 2021

Seems great! I'll have to get used to the String @NotNull [] notation but saw the same being used in Google Guava.

Heads up btw, I'm planning this in for 1.4.0 as I should release 1.3.0 very soon and I'll have to merge in another branch to master for that release still. I hope that won't cause too many conflicts (it's mainly a smaller refactoring in the YAML file resource)

@Sxtanna
Copy link
Contributor Author

Sxtanna commented Oct 17, 2021

I'll have to get used to the String @NotNull [] notation but saw the same being used in Google Guava.

Yeah, and especially considering that only marks the array object itself as not null, if you want to say both the object is not null AND the array should only contains non null values it's @NotNull String @NotNull [].

What a beauty. 😭

@Sxtanna Sxtanna marked this pull request as ready for review October 17, 2021 16:35
@ljacqu
Copy link
Member

ljacqu commented Nov 6, 2021

Thanks for doing this! I'm clicking through the files and it looks great. I have two questions

  • do you think @ParametersAreNonnullByDefault would be picked up by IntelliJ enough that sometime we could revert to using that and only @Nullable? (Not to do it in this PR, don't get me wrong, just interested in an opinion)
  • the new annotations project is now packaged into the project and no longer set as provided. Do you know if that impacts the JAR size by much?

@Sxtanna
Copy link
Contributor Author

Sxtanna commented Nov 7, 2021

do you think @ParametersAreNonnullByDefault would be picked up by IntelliJ enough that sometime we could revert to using that and only @Nullable? (Not to do it in this PR, don't get me wrong, just interested in an opinion)

Judging by their consistent unwillingness, probably not:
JetBrains/java-annotations#60
JetBrains/java-annotations#18

the new annotations project is now packaged into the project and no longer set as provided. Do you know if that impacts the JAR size by much?

This was actually a mistake, none of the annotations have a runtime retention policy anyway. Setting the dependency to provided is perfectly fine.

(for reference though, shading the annotations adds ~26KB to the jar size)

# Conflicts:
#	src/main/java/ch/jalu/configme/resource/MapNormalizer.java
#	src/main/java/ch/jalu/configme/resource/PropertyPathTraverser.java
#	src/main/java/ch/jalu/configme/resource/YamlFileReader.java
#	src/main/java/ch/jalu/configme/resource/YamlFileResource.java
#	src/main/java/ch/jalu/configme/resource/YamlFileResourceOptions.java
@ljacqu
Copy link
Member

ljacqu commented Jun 4, 2022

Thanks for addressing everything! Taking this over now, and will hopefully get used to the new annotations concept 😄✨

@ljacqu ljacqu merged commit 9dbf613 into AuthMe:master Jun 4, 2022
@ljacqu ljacqu added this to the 1.4.0 Release milestone Jun 4, 2022
@ljacqu ljacqu mentioned this pull request Jun 4, 2022
@ljacqu ljacqu added the kotlin label Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants