feat: add new backend to support DOIs - Rclone 1.65.2#2
Conversation
This reverts commit 4cea7cc.
leafty
force-pushed
the
leafty/exp-doi-zenodo-1-65
branch
from
f178a53 to
1bb8a99
Compare
leafty
force-pushed
the
leafty/exp-doi-zenodo-1-65
branch
from
1bb8a99 to
63af447
Compare
leafty
changed the title
leafty
force-pushed
the
leafty/exp-doi-zenodo-1-65
branch
from
aeee2e8 to
d93097f
Compare
olevski
left a comment
olevski
left a comment
There was a problem hiding this comment.
I added a few questions / suggestions. Feel free to ignore since this will be reviewed again by the rclone maintainers. So I did not want to make suggestions that potentially the maintainers would ask you to undo.
| // DataverseDatasetVersion is the representation of a dataset version | ||
| type DataverseDatasetVersion struct { | ||
| LastUpdateTime string `json:"lastUpdateTime"` | ||
| Files []DataverseFile `json:"files"` |
There was a problem hiding this comment.
question: I think you are missing some of this on this and other list or optional fields.
| Files []DataverseFile `json:"files"` | |
| Files []DataverseFile `json:"files,omitempty"` |
There was a problem hiding this comment.
The same goes for the invenio stuff types. But it probably does not really matter because I think you only use these to deserialize json stuff into the structs and you never really serialize these structs to json. Right?
There was a problem hiding this comment.
- I don't use the classes for sending requests
- I don't know if this is even how the API works.
omitemptyshould be there only if this is also valid API-wise.
| return metadata.Metadata.Title, nil | ||
|
|
||
| } | ||
| return "<unknown title>", nil |
There was a problem hiding this comment.
question: shouldn't you return an error if you cannot find the title?
There was a problem hiding this comment.
Maybe. It is still pretty obvious from <unknown title> that something wrong happened.
There was a problem hiding this comment.
Ok I guess the rclone authors can decide how to address this. If I saw a function and it returned an error and something and if the error is nil then I would expect that that "something" should be normal. In this situation that is not the case.
| for _, opt := range opts.Options { | ||
| k, v := opt.Header() | ||
| fs.Logf(o, "header '%s' = '%s'", k, v) | ||
| } |
There was a problem hiding this comment.
question: Is there a risk of leaking sensitive information from printing this out?
There was a problem hiding this comment.
Most of the logging statements should be removed soon.
| // List the files contained in the DOI | ||
| func (f *Fs) listInvevioDoiFiles(ctx context.Context) (entries []*Object, err error) { | ||
| // Use the cache if populated | ||
| cachedEntries, found := f.cache.GetMaybe("files") |
There was a problem hiding this comment.
question: Do we need to worry about accessing stuff that is too stale from the cache? Do we have some way to invalidate the cache? Or is this unnecessary and once we have something in the cache we can be certain that it does not need to be updated.
There was a problem hiding this comment.
Or is this unnecessary and once we have something in the cache we can be certain that it does not need to be updated.
Correct, the file list from a published dataset should not change. It may change if a new version is published or if file hosting is updated.
There was a problem hiding this comment.
So if we can just put some simple TTL on the cache it may be a good idea. Because it is not like the dataset can NEVER change. But again we can just wait on the review from the rclone authors here.
There was a problem hiding this comment.
It's 5 minutes since the entry was last used. So, technically the entry may never expire if it is always in use.
There was a problem hiding this comment.
Otherwise, it will be fine in most reasonable cases.
leafty
force-pushed
the
leafty/exp-doi-zenodo-1-65
branch
from
fdd182d to
f476e0f
Compare
leafty
force-pushed
the
leafty/exp-doi-zenodo-1-65
branch
from
c858c78 to
a1647a7
Compare
2 participants
Same as #1, but rebased onto 1.65.2