-
-
Notifications
You must be signed in to change notification settings - Fork 20
[bis] Fix misimport of ktor variable in common module #110
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
base: main
Are you sure you want to change the base?
[bis] Fix misimport of ktor variable in common module #110
Conversation
rfc2822
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of having a manually maintained annotation for Ktor dependencies. It looks error-prone and if we just forget a single thing (or forget to not use such annotated methods in DAVx5 code), everything breaks.
So considering that we want to transition to Ktor anyway, I think we can keep the ktor code for simplicity and just not exclude the ktor library in DAVx5.
And then hopefully make progress to move everything to ktor …
Maybe we just specify it in the comments? To be sure. Because with other functions it's clear (for example, they use a ktor reference in the arguments), but for others (such as they use |
However we'd then have to remove it again when we drop okhttp … I think we should keep ktor usage in common code as little as possible, but it's acceptable. We just need to know that Ktor must not be excluded in gradle. So maybe mention that in the README? That dav4jvm is currently in a rewrite phase to Ktor and that ktor must not be excluded. We can then update it when we remove okhttp from the dependencies. |
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
|
I've updated everything, and added a note to the README. Maybe the title of the PR should be updated now |
rfc2822
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a comment to the Ktor issue: #72 (comment)
| const val Accept: String = "Accept" | ||
|
|
||
| /** [RFC4229 Section-2.1.5](https://datatracker.ietf.org/doc/html/rfc4229#section-2.1.5) */ | ||
| const val AcceptEncoding: String = "Accept-Encoding" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. There are some references (especially in DavResource), we can replace them right now so that the new constants are actually used.
We can replace header names in other places whenever we change code in those places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can replace them right now so that the new constants are actually used.
So we want to ignore completely that ktor's HttpHeaders exists, and use our own, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now I'm confused, I thought ktor HttpHeaders is rather an internal class.
But it's part of the API, so we can of course use HttpHeaders everywhere, especially as we have given up the strict separation of okhttp and ktor.
So we won't need this class (HttpHeaderNames) at all because ktor HttpHeaders also defines ETag and ScheduleTag.
HttpHeaders: created our own HTTP headers constants file.@UsesKtor:ContentType, but it's a bit too complex to implement it on our own, so at least annotate it.