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: add nullness annotations on public API (WIP) #1009

Merged
merged 31 commits into from
Oct 24, 2024

Conversation

greyhairredbear
Copy link
Contributor

@greyhairredbear greyhairredbear commented Jul 30, 2024

Opening this draft PR to see CI results for javadoc & collect early feedback

Open items (remaining open items are to be delivered with #1160)

  • domain
  • impl related to domain package
  • function
  • impl related to function package
  • score
  • score.analysis
  • score.buildin
  • score.calculator
  • score.constraint
  • score.director
  • score.stream
  • score.stream.bi
  • score.stream.common
  • missing annotations in score.stream.common.ConstraintCollectors
  • score.stream.penta
  • score.stream.quad
  • score.stream.tri
  • score.stream.uni
  • impl related to score package
  • solver
  • (re)check if all impl related to solver package has been annotated
  • config
  • timefold-solver-benchmark
  • timefold-solver-test
  • check format :)
  • collect open todos and resolve together with maintainer

Skipped (for now):

  • annotation attributes (e.g. PiggybackShadowVariable.shadowVariableName) - not sure if this would be valuable. IDE complains "Attribute value must be constant" when trying to assign null here. If there is value in this because the solver internally uses these attributes, I'd be ok with adding these too.
  • deprecated classes and methods

@triceo
Copy link
Contributor

triceo commented Jul 30, 2024

Thanks for the PR, @greyhairredbear!
I'll review once the PR is undrafted.

@triceo triceo self-requested a review July 30, 2024 17:57
@triceo triceo self-assigned this Jul 30, 2024
@triceo triceo added java Is related to Java code. kotlin Is related to Kotlin code. labels Jul 30, 2024
@triceo
Copy link
Contributor

triceo commented Jul 31, 2024

FYI @greyhairredbear:

  • The code coverage metric from Sonar is likely not relevant in this case. You're mostly be touching the API code here, so don't let it discourage you.
  • Please also think of the config package. Even though it's technically not api, people are likely to use it and we treat it as public API.
  • There are APIs in the timefold-solver-benchmark and timefold-solver-test module too.

Copy link

sonarqubecloud bot commented Aug 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 70%)

See analysis details on SonarCloud

@greyhairredbear
Copy link
Contributor Author

@greyhairredbear My other PR is ready to go and I need to merge it before the end of the week. Is there anything else you intend to do here before marking this as "Ready for review"?

(The actual review is already done.)

Sorry, I thought this was waiting for #1149 to be completed - but I just saw some tests there are still failing because Java 21 is not supported.

I guess it's ok to merge this, so I'll undraft it. Please don't hesitate to contact me if there are any issues related to this, I'd be glad to help and don't want to block anybody.

I'll open a new PR with the remaining tasks and reference it from here once I start working on the remaining changes.

@greyhairredbear greyhairredbear marked this pull request as ready for review October 24, 2024 06:47
@triceo
Copy link
Contributor

triceo commented Oct 24, 2024

Thank you, @greyhairredbear! I will merge this later today.
As agreed, I'm looking forward to the rest of this great work on/before Nov 3rd!

@triceo triceo added this to the 1.16.0 milestone Oct 24, 2024
@triceo triceo merged commit 34dded5 into TimefoldAI:main Oct 24, 2024
25 of 27 checks passed
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@greyhairredbear greyhairredbear deleted the feat-nullness-annotations branch October 24, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java Is related to Java code. kotlin Is related to Kotlin code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants