8000
Skip to content

Add java.util.Calendar to DenyListedApiDetector#351

Merged
ZacSweers merged 3 commits intoslackhq:mainfrom
jbduncan:issues/347
Feb 17, 2025
Merged

Add java.util.Calendar to DenyListedApiDetector#351
ZacSweers merged 3 commits intoslackhq:mainfrom
jbduncan:issues/347

Conversation

@jbduncan
Copy link
Copy Markdown
Contributor
@jbduncan jbduncan commented Feb 7, 2025

Summary

This PR fixes #347, aiming to catch usages of java.util.Calendar and suggest java.time instead.

Requirements (place an x in each [ ])

The following point can be removed after setting up CI (such as Travis) with coverage reports (such as Codecov)

  • I've written tests to cover the new code and functionality included in this PR.

The following point can be removed after setting up a CLA reporting tool such as cla-assistant.io

@salesforce-cla
Copy link
Copy Markdown
salesforce-cla bot commented Feb 7, 2025

Thanks for the contribution! Before we can merge this, we need @jbduncan to sign the Salesforce Inc. Contributor License Agreement.

@jbduncan
Copy link
Copy Markdown
Contributor Author
jbduncan commented Feb 7, 2025

CLA signed!

Comment thread gradle/libs.versions.toml
autoService-ksp = "dev.zacsweers.autoservice:auto-service-ksp:1.2.0"
junit = "junit:junit:4.13.2"
kotlin-metadata = { module = "org.jetbrains.kotlin:kotlin-metadata-jvm", version.ref = "kotlin" }
ktfmt = { module = "com.facebook:ktfmt", version.ref = "ktfmt" }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you restore this? Curious why it was removed as we use this for formatting

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Certainly. I was curious too, because my IntelliJ IDEA tells me that it's unused, and removing it doesn't cause ./gradlew check to fail.

I think it's because ktfmt under [versions] is used but ktfmt in [libraries] isn't, but I'm happy to be corrected on this.

Anyway, seeing your other comment, I've reverted the change locally and will force-push soon.

*/
internal class DenyListedApiDetector : Detector(), SourceCodeScanner, XmlScanner {

override fun getApplicableUastTypes() = CONFIG.applicableTypes()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you clean up some of these extraneous refactorings? Would like to keep this addition limited to just the new Calendar entry unless there's some functional change required to make that work

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Certainly! I've removed the commits that introduced the extraneous refactorings locally and will force-push soon.

Copy link
Copy Markdown
Collaborator
@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Thanks!

@ZacSweers ZacSweers added this pull request to the merge queue Feb 17, 2025
Merged via the queue into slackhq:main with commit a29179f Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DenyListedApiDetector.kt seems to be missing java.util.Calendar

2 participants

0