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.

Sign up for GitHub

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

Jump to bottom

Include function annotations in request attribute #759

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Include function annotations in request attribute #759

wants to merge 11 commits into from

Conversation

dewantawsif
Copy link
Contributor

Closes #751

🤔 DOD Checklist

  • I did all relevant changes to the documentation and the changelog.

@dewantawsif dewantawsif requested a review from Foso as a code owner January 8, 2025 15:53
@github-actions github-actions bot added documentation Improvements or additions to documentation ktorfit-ksp ktorfit-lib labels Jan 8, 2025
@dewantawsif
Copy link
Contributor Author

@Foso sorry for being impatient but can I have this looked at and merged?

Foso
Foso requested changes Feb 3, 2025
View reviewed changes
Copy link
Owner

@Foso Foso left a comment

Choose a reason for hiding this comment

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

Hi @dewantawsif thank you for your pull request! I looked at it, but for me it's not working. I can only find the annotations from Ktorfit and nothing else. Also please write a test for the code generation

Screenshot 2025-02-03 at 19 23 47

@dewantawsif
Copy link
Contributor Author

Hi @Foso I've did my own testing and even looked at the generated code and everything seems to be working fine. But I did notice that you've added the annotation to getUser while a call is made to getUserResponse in the main function

Ktorfit/sandbox/src/jvmMain/kotlin/de/jensklingenberg/ktorfit/demo/JvMMain.kt

Line 65 in 737ab5c

val user = api.getUserResponse()

If this isn't the issue please let me know the steps you took to test the code so I could try to reproduce.

@dewantawsif
Copy link
Contributor Author

I changed the file name 😕

@Foso
Copy link
Owner

Foso commented Feb 12, 2025

@dewantawsif Your code is working, but i'm thinking about if we should limit the functionality to non-Ktorfit annotations. Currently all information of the Ktorfit annotations gets added to the RequestBuilder and to the attributes. I think that unnecessarily bloats the source files of the implementation class. When someone creates a ClientPlugin and has access to the HTTPRequestBuilder they can already use it to access things like request method, headers, etc .

@dewantawsif
Copy link
Contributor Author

Yeah that makes sense. I'll exclude the following ktorfit annotations: GET, POST, PUT, DELETE, HEAD, OPTIONS, PATCH, Headers, FormUrlEncoded, Multipart and Streaming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Foso Foso Foso requested changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation ktorfit-ksp ktorfit-lib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Custom annotations support like Retrofit?
2 participants
@dewantawsif @Foso