feat!: improve file client implementation #1175
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1175 +/- ##
==========================================
+ Coverage 76.33% 76.66% +0.33%
==========================================
Files 80 80
Lines 16827 16813 -14
Branches 1616 1618 +2
==========================================
+ Hits 12845 12890 +45
+ Misses 3952 3893 -59
Partials 30 30 ☔ View full report in Codecov by Sentry. |
JKRhb
force-pushed
the
fix-file-client
branch
from
734a307 to
8df0d48
Compare
|
Had to apply some Windows-related fixes, the PR should now be ready for review :) |
danielpeintner
left a comment
danielpeintner
left a comment
There was a problem hiding this comment.
LGTM.
Added 2 comments...
relu91
left a comment
relu91
left a comment
There was a problem hiding this comment.
Looks good just minor comments below. I would wait before merging for the point that Daniel made on the .gitignore.
packages/binding-file/README.md
Outdated
| let wotHelper = new Helpers(servient); | ||
| wotHelper | ||
| .fetch("file://TD.jsonld") | ||
| .fetch("file:///TD.jsonld") |
There was a problem hiding this comment.
Should we change this example using the new official requestThingDescription function?
There was a problem hiding this comment.
Good point, I would probably add that to #1166, though, alongside the rest of the file-related implementation of the new method.
There was a problem hiding this comment.
Ah, okay, nevermind, as #1166 got already merged ;) Then I will add it to this PR instead :)
There was a problem hiding this comment.
I added an initial update of the example in 1b79e91, haven't tested it yet, though.
There was a problem hiding this comment.
I now performed some tests and actually found a few bugs which I fixed in 3793b6e.
|
|
||
| public async readResource(form: Form): Promise<Content> { | ||
| const filePath = fileURLToPath(form.href); | ||
| const contentType = form.contentType ?? ContentSerdes.DEFAULT; |
There was a problem hiding this comment.
I know this is correct behavior, but it might be error-prone... people will try to serve files without expressing the content-type thinking that the client could figure it himself. Anyhow, just a comment no action is required here.
There was a problem hiding this comment.
Maybe we can add a debug comment "saying that"... a la
... no contentType set, picked JSON by default
There was a problem hiding this comment.
Tried to incorporate this in 14c1780, let me know if that works for you :)
In general, I was wondering if we should maybe move the default Content-Type determination to the Form class (maybe as part of a get method) to have a consistent behavior across the codebase. I would make an initial proposal in a follow-up PR that we could discuss there if that's okay for you.
JKRhb
force-pushed
the
fix-file-client
branch
from
14c1780 to
0780ba4
Compare
Co-authored-by: danielpeintner <[email protected]>
3 participants
As a spinoff from #1166, this PR extends the file client implementation with support for the
writepropertyoperation and improves the handling offile://URLs. As the determination of the Content-Type from the file extension was somewhat deviating from the standard (as this ignored the default Content-Type implied by a TD form), I removed it.This last aspect might be a bit controversial, but I think this should be the better approach to have an implementation that is extendable with additional Content-Types by users who can simply register them with the
ContentSerdes. However, I looking forward to feedback not only in this regard :)