Use correct CIDs #41
Use correct CIDs #41
Conversation
corrideat
force-pushed
the
1927-design-implement-contract-deletion-op_contract_delete-1
branch
from
4023661 to
d7abc7b
Compare
src/deploy.ts
Outdated
| @@ -13,11 +13,11 @@ export async function deploy (args: string[]) { | |||
| const json = JSON.parse(Deno.readTextFileSync(manifestPath)) | |||
| const body = JSON.parse(json.body) | |||
| const dirname = path.dirname(manifestPath) | |||
| toUpload.push(path.join(dirname, body.contract.file)) | |||
| toUpload.push('t|' + path.join(dirname, body.contract.file)) | |||
There was a problem hiding this comment.
What is this t| and m| stuff? Can you add a comment explaining?
There was a problem hiding this comment.
It's a prefix because we need to know what kind of CID should be used.
src/upload.ts
Outdated
| for (const filepath_ of files) { | ||
| let type = multicodes.RAW | ||
| let filepath = filepath_ | ||
| if (filepath_[1] === '|') { | ||
| switch (filepath_[0]) { | ||
| case 'r': | ||
| // raw file type | ||
| break | ||
| case 'm': | ||
| type = multicodes.SHELTER_CONTRACT_MANIFEST | ||
| break | ||
| case 't': | ||
| type = multicodes.SHELTER_CONTRACT_TEXT | ||
| break | ||
| default: | ||
| throw new Error('Unknown file type: ' + filepath_[0]) | ||
| } | ||
| filepath = filepath_.slice(2) | ||
| } | ||
| const entry = await createEntryFromFile(filepath, type) |
There was a problem hiding this comment.
Is it not a concern that a user might use chel upload on a file that has | as the 2nd character?
There was a problem hiding this comment.
Files can't have | on Windows (one of the reasons I chose it; maybe the same goes for macOS, but I can't remember). However, for such files, they could be prefixed as needed. E.g., if the filename is ||, one could use r|||
There was a problem hiding this comment.
On POSIX, only 0x00 (and maybe /) is an invalid character in file name, but using 0x00 for this purpose looked wrong, as it's difficult to type in a terminal.
There was a problem hiding this comment.
OK, then do you make sure that everywhere upload is called the passed in files are prefixed?
And if they're all prefixed, what's the point of check on line 20? Instead, we should probably throw an exception if | isn't found.
There was a problem hiding this comment.
The intent was to allow the command to be used from the command line without a prefix (in which case a raw id would be used)
There was a problem hiding this comment.
Then we can invent some other flag or function for that.
There was a problem hiding this comment.
But a flag wouldn't work very well because the upload command works with multiple files.
There was a problem hiding this comment.
We cannot have ambiguity here because such ambiguity would lead to unexpected behavior and bugs.
With a flag the user would be able to specify the type for all passed in files, and call the command again if they want to specify a different type.
There was a problem hiding this comment.
Sure, but I don't really see the ambiguity here as | is pretty rare as a file name, and I expect that having a file called r|XX and another one called XX is even rarer.
There was a problem hiding this comment.
I agree, it's rare, but not so rare that it never happens. On macOS | is a valid filename character. If we can fix this now and forget about it, we don't need to ever worry about it in the future.
corrideat
force-pushed
the
1927-design-implement-contract-deletion-op_contract_delete-1
branch
from
69e7d92 to
405d425
Compare
EDIT (by @taoeffect): see also: multiformats/multicodec#369