Skip to content

fixes ipfs uri to generalized format#54

Merged
pipermerriam merged 3 commits intomasterfrom
ipfsUriFix
Mar 2, 2017
Merged

fixes ipfs uri to generalized format#54
pipermerriam merged 3 commits intomasterfrom
ipfsUriFix

Conversation

@mhhf
Copy link
Copy Markdown
Collaborator

@mhhf mhhf commented Feb 2, 2017

No description provided.

@pipermerriam
Copy link
Copy Markdown
Member

Given this is backwards compatible this feels fine to merge and backfill into the v1 spec. Any objections @iurimatias @tcoulter @VoR0220

@VoR0220
Copy link
Copy Markdown
Collaborator

VoR0220 commented Feb 2, 2017

The only objection I would have is to give a detailed reasoning for the regex expression as the way it is. Other than that I'm good.

@tcoulter
Copy link
Copy Markdown
Contributor

tcoulter commented Feb 2, 2017

I don't have an intuitive understanding of other URLs. If there's a URL that's different than ipfs://<multihash>, I may not currently process it correctly as I rely on that multihash specifically. I would love a detailed reasoning as well.

@pipermerriam
Copy link
Copy Markdown
Member

@tcoulter reasoning comes from this post from the go-IPFS repo: ipfs/kubo#1678 and specifically juan's comment here ipfs/kubo#1678 (comment)

@VoR0220
Copy link
Copy Markdown
Collaborator

VoR0220 commented Feb 2, 2017

...okay...then let's put that in the spec.

@pipermerriam
Copy link
Copy Markdown
Member

@VoR0220 this PR gets the written version of the spec inline with what is already present in the jsonschema version of the spec.

The result of this will be that both the written version and the jsonschema version will allow all of

  • ipfs://Qm...
  • ipfs:/Qm... (previously disallowed in the written version)
  • ipfs:Qm... (previously disallowed in the written version)
  • ipfs://Qm.../some/path
  • ipfs:/Qm.../some/path (previously disallowed in the written version)
  • ipfs:Qm.../some/path (previously disallowed in the written version)

So 👍 or 👎

@pipermerriam
Copy link
Copy Markdown
Member

@VoR0220 I think I mis-understood your comment. Are you suggesting we add that link to the specification to indicate where the URI format comes from?

@VoR0220
Copy link
Copy Markdown
Collaborator

VoR0220 commented Feb 3, 2017

Not add the link because that can disappear, moreso describe in plaintext where the URI format comes from.

@pipermerriam
Copy link
Copy Markdown
Member

pipermerriam commented Feb 5, 2017 via email

@mhhf
Copy link
Copy Markdown
Collaborator Author

mhhf commented Feb 6, 2017

added a short explanation, hope its enough.

Should we also shorten the regex to ^ipfs:.*$? since its technically the same thing as saying ^ipfs:/?/?.*$ while also more readable?

@pipermerriam pipermerriam merged commit 7f96ba0 into master Mar 2, 2017
@pipermerriam pipermerriam deleted the ipfsUriFix branch March 2, 2017 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants