slackhq / slack-lints

A collection of custom Android/Kotlin lint checks we use in our Android and Kotlin code bases at Slack.
Apache License 2.0
229 stars 13 forks source link

Allow unit return type for suspend functions #293

Closed oguzcelik closed 1 month ago

oguzcelik commented 3 months ago

Summary

This pr aims to fix the lint rule for retrofit usage. Solves this issue

For suspend functions it's a valid case to return Unit.

Requirements (place an x in each [ ])

Unfortunately the contributing guidelines and Contributor license agreement links are not working.

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

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

salesforce-cla[bot] commented 3 months ago

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

ZacSweers commented 3 months ago

Let's make this more intentional. Could we either add an opt-in annotation like @AllowUnitResponse with a doc on it explaining contexts where it makes sense? Blanket disabling this feels like the wrong move when the goal is to make developers more intentional.

oguzcelik commented 3 months ago

Would you want to make the docs more explicit ? Something like:

/**
 * Annotation to allow suspend Retrofit functions to return Unit.
 * 
 * This annotation is used to indicate that a suspend Retrofit function is allowed to have 
 * a return type of [Unit]. This might be necessary for certain HTTP methods where the 
 * response body is not significant. For instance:
 * 
 * - Methods like `@POST`, `@PUT`, or `@DELETE` may logically return `Unit` if 
 *    the client is only concerned with the success/failure of the operation and not the 
 *    actual response body.
 * 
 * Invalid Use Cases:
 * - `@GET`: Typically, `@GET` methods should return a meaningful response object as 
 *    they are intended to retrieve data.
 */
ZacSweers commented 1 month ago

Sorry @oguzcelik but you've not signed our CLA, so I cannot accept this PR or finish it for you. Instructions were commented here.

oguzcelik commented 1 month ago

Hey @ZacSweers 🖖 hope you enjoyed your sabbatical 🥳

I had already mentioned in the pr description that the CLA link unfortunately doesn't work.

Here's a video in case you are wondering what I mean

https://github.com/user-attachments/assets/1496f452-153b-4773-81f3-f7a5e1eb6540

Please let me know once the cla link and the contributing guidelines links are fixed and I'll try to get back to this to open another pr 👍

ZacSweers commented 1 month ago

Please use the one in the comment I linked above? The template is unfortunately old but the comment from the salesforce bot should be working