feat: show which link does not exist on ipfs.cat #1270
Conversation
There was a problem hiding this comment.
Thanks for the contribution @youngnicks. This PR is missing, however, the case for when on a path /a/b/c, c doesn't exist on b, it is only covering the whole path a/b/c.
|
Thanks for the feedback @diasdavid. I'll work on getting that functionality added as well as appropriate tests. |
|
@diasdavid I added logic to determine which node in the path is missing and updated the error message. I want to get some feedback on how I did this as it could have performance hits on nodes containing lots of files. If this is acceptable, I will add a test case for the scenario when part of the path doesn't exist. If this isn't acceptable, I'm going to have to put logic in a higher function to determine where the missing path is. |
| expect(err).to.exist() | ||
| expect(message).to.eql( | ||
| 'no link named "dummy" under QmPZ9gcCEpqKTo6aq61g2nXGUhM4iCL3ewB6LDXZCtioEB' | ||
| ) |
There was a problem hiding this comment.
This feature is still not complete. Please add tests for multiple levels of nestedness (i.e /a/b/c/d) and you will see that after the first level, this PR fails.
There was a problem hiding this comment.
Thanks, I was more asking if this would be an acceptable location for this logic, knowing that it could possibly have a performance hit when there is a large amount of files on a node.
What should the error message show when a middle level is missing? I am thinking the message should be no link named "c/d" under a/b to provide more context to as to what the requested file is.
|
@diasdavid I added an additional test for nestedness. Both tests are passing. Is the format of the nested error correct, or should it be saying something different? |
|
@diasdavid After playing around with |
|
@pgte mind reviewing this PR and help @youngnicks get it completed? |
There was a problem hiding this comment.
The error message is much better than before, but, thanks to @youngnicks work here, I think there's the opportunity here to present a completely unambiguous error message.
test/cli/files.js
Outdated
| .catch((err) => { | ||
| const message = err.stderr.match(/^Error:(?: Failed to cat file: Error:)? (.*)$/m)[1] | ||
| expect(message).to.eql( | ||
| 'no link named "wrong-dir" under QmegvLXxpVKiZ4b57Xs1syfBVRd8CbucVHAp7KpLQdGieC' |
There was a problem hiding this comment.
I would expect the error to say something like either:
"no file named "wrong-dir" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs"or"no file named "init-docs/docs/wrong-dir" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2"or
Would any of these be possible?
There was a problem hiding this comment.
Notice that I'm using the term file and not link, on purpose ;)
| const filterFile = (file) => { | ||
| if (file.path === ipfsPath.substring(0, file.path.length)) { | ||
| if (file.depth > bestMatch.depth) { | ||
| bestMatch = file |
There was a problem hiding this comment.
Instead of replacing, could we retain the complete path of the best match to support more relevant error messages? (See my previous comment)
There was a problem hiding this comment.
I think replacing here is still okay as the depth tells us how many nodes bestMatch matches and we can use pathComponents to build the path. This is essentially what I was doing earlier on in the pull request. I thought of defining if a directory or file was missing, but it wasn't part of the spec so I left it the way it was.
We could have another scenario where a user tries to specify a file under a file thinking it is a directory. What do you think of these errors?
Missing directory
no directory named "wrong-dir" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs
Missing file
no file named "does-not-exist" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs
File instead of directory
"actually-a-file" is a file not a directory under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs
There was a problem hiding this comment.
@youngnicks this would be even better! 👍
|
I modified the error messages to be more specific and added a new one which informs the user when a file is trying to be used as a directory. @pgte Can you please take a look at this and let me know if you would like any other changes? |
|
@youngnicks Error messages look great! To move on, I guess interface-ipfs tests need to be updated, right? |
|
@pgte Thanks! I already submitted a pull request for |
daviddias
changed the title
|
Hi! js-ipfs master just got a whole new set of automated tests with #2528, #2440 and also running some of the test suites from our early testers (hi5 to @achingbrain for setting it all up!). Would you mind rebasing the master branch on this PR to ensure it runs all the latest tests? Thank you! |
|
Closing as this is stale and there have been significant changes since this was created |
Closes #1145.
For
interface-ipfs-coretest to pass, the corresponding test there needs to be updated to be similar to below as one version of running tests has an error message prepended withFailed to cat file: Error:, causing the test to fail.