Feat/boxes data #671
Feat/boxes data #671
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
scheidtdav
left a comment
scheidtdav
left a comment
There was a problem hiding this comment.
Could you check the two lint alerts @jona159
app/routes/api.boxes.data.ts
Outdated
| return `opensensemap_org-${action}-${encodeURI( | ||
| encodeURI(params.join('-')), |
There was a problem hiding this comment.
that is really strange. Do we really want to keep that?
If the first encoding pass would result in invalid characters, wouldn't the second pass also potentially produce invalid characters and both should be avoided entirely?
Vice-versa, if the first one already takes care of encoding, why bother doing it again? It just makes the result longer and less readable.
Maybe this was added by mistake by @ubergesundheit or there is something we don't know :-)
There was a problem hiding this comment.
Since it is just creating a filename i just removed the first encoding for now...
| try { | ||
| const collected = collectParameters(request, params) | ||
| if (collected instanceof Response) return collected | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars |
There was a problem hiding this comment.
is there a reason to disable the warning rather than just not unpacking the vars from collected?
There was a problem hiding this comment.
Hm i dont think i made changes to this file (other than formatting). I removed the deviceId from the destructured object and the comment here as deviceId was unused
|
|
||
| const normalizedParams = { | ||
| phenomenon: getParam(['phenomenon']), | ||
| boxId: getParam(['boxId', 'boxid']), // boxId in docs, but effectively boxid was used, so support both? |
There was a problem hiding this comment.
I'd say we should either support all fallbacks of the original implementation or just stick to boxId in the correct spelling. If we do add case-insenstivness here, we would also have to do it elsewhere, don't you think?
There was a problem hiding this comment.
I adjusted to use only boxid as this is what the old app used (docs were falsly stating boxId here). I would agree we should stick with one case and not add case-insensitivness everywhere now
Type of Change
Implementation
Checklist
devbranchAdditional Information