-
Notifications
You must be signed in to change notification settings - Fork 54
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.
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
WIP: or combinator improvement #143
base: master
Are you sure you want to change the base?
Conversation
@@ -472,7 +506,7 @@ describe('CID', () => { | |||
const hash = await sha256.digest(textEncoder.encode('abc')) | |||
const cid = CID.create(1, 112, hash) | |||
|
|||
const parsed = CID.parse(cid.toString(base58btc)) | |||
const parsed = /** @type {CID} */(CID.parse(cid.toString(base58btc))) |
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.
any idea why it's failing to infer 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.
It's not failing it's just return type is null|CID
this manually refines it to CID
, which is something TS allows.
], | ||
"exclude": [ | ||
"vendor", | ||
"node_modules" | ||
"node_modules", | ||
"node_modules/multiformats" |
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.
this is from the ts-use test installation? or something else?
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 think this is no longer necessary, I was running into a problem where all imports were picked up from node_modules/multiformats
as opposed to from src
in the end mapped paths is what fixed that. This is just artifact of my trying to figure out the way.
Yeah, that is pretty silly. Although maybe the clean-up in there is worth it, it's quite nice now, aside from that. I don't mind your |
I don't know if it's not the long list of things doing |
I think I'll make few more changes to this pr specifically:
|
@Gozala : what are the next steps here? Is this work being carried forward? |
(question from triage) @Gozala would it be less disruptive if we update the interface instead? |
I’m not sure how would you do that without going and changing an implementation to comply with it, unless you ts-ignore it away.
Sorry I missed that comment. We can either take this as is which is better that what we have and addresses the issue at hand, or I can try to find time to make changes discussed. Those changes could be future improvements however as they would just add more functions. |
Attempt to fix #141
I like that this patch gets rid of
UnibaseDecoder
all together which simplifies things a bit. However I do not like sillycomposed
getter it introduces just so that arrayreduce
will not get confused by the fact that argument is eitherDecoder<B, P>
orCompositeDecoder<P>
.I think it would be better to either
base32.decoder.or(base64.decoder).or(base58.decoder)...
. Not ideal but I'm guessing number of decoders isn't going to be really big anyway.export const fromTable = (decoders) => new CompositeDecoder(decoders)
so that one could simply provide table when needed as opposed to building up decoder withor
operator.P.S.: I originally left this out, because
or
works with composed and single base decoders, table on the other hand requires only single base decoders.