-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! Been needing this a couple of times so I think this is useful addition.
cb(err, exists) | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's converge repoExists and ifRepoExists and make the converged function async
(with callbacks and a promisify wrapper), the reason why it needs to be async is because the repo might be over a wire (like a S3 bucket).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aphelionz , how is the development of this feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diasdavid Gonna push some more commit(s) today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you need any help :)
@aphelionz could you also please follow the commit message format? https://github.com/ipfs/community/blob/master/js-project-guidelines.md#commit-message-format (I know this might feel annoying, but it helps us create awesome changelogs! :)) |
@@ -62,6 +63,7 @@ class IPFS { | |||
this.bitswap = components.bitswap(this) | |||
this.ping = components.ping(this) | |||
this.pubsub = components.pubsub(this) | |||
this.repoExists = utils.repoExists(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a new utility function, we could just assign this.repoExists
to repoInstance.exists
and all should work. Would be less duplicated code + tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give that a shot. The test should still run against this.repoExists though, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would say we should still test it, to be sure. Better safe than sorry
Is there still an interest in this feature? |
I've created an extra utility function based on @dignifiedquire and my conversation in #773
Happy to fix any issues or write up any required documentation here.
Also, we should discuss what we should do with
utils.ifRepoExists
andutils.repoExists
which are very similar but very differently behaving functions.