Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not throw? #25

Closed
alanshaw opened this issue Feb 28, 2018 · 4 comments
Closed

Do not throw? #25

alanshaw opened this issue Feb 28, 2018 · 4 comments
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up

Comments

@alanshaw
Copy link
Member

mafmt.HTTP.matches('/ip4/127.0.0.1/tcp/90/http/baz.jpg') currently throws. Would it be better for consumers of this library if it just returned false?
@daviddias
Copy link
Member

👍

@vmx
Copy link
Member

vmx commented Feb 28, 2018 •
edited
Loading

As mentioned in multiformats/js-multiaddr#53 I think it should return true.

@alanshaw
Copy link
Member Author

That specific example aside, what I'm asking is: should any bad multiaddr that causes a throw should be caught and return false? I feel like it should.

e.g. mafmt.HTTP.matches('junk') -> false

@daviddias daviddias added status/ready Ready to be worked help wanted Seeking public contribution on this issue exp/novice Someone with a little familiarity can pick up P2 Medium: Good to have, but can wait until someone steps up labels May 30, 2018
mkg20001 added a commit to mkg20001/js-mafmt that referenced this issue Jan 11, 2019
@mkg20001
fix: do not throw if multiaddr str is invalid, fixes multiformats#25
de15ed1
@mkg20001
Copy link
Contributor

🎉 PR #36

mkg20001 added a commit to mkg20001/js-mafmt that referenced this issue Jan 17, 2019
@mkg20001
fix: do not throw if multiaddr str is invalid, fixes multiformats#25
a5ef750
@ghost ghost removed the status/ready Ready to be worked label Jan 25, 2019
@alanshaw alanshaw mentioned this issue Jul 15, 2019
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@vmx @alanshaw @daviddias @mkg20001