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.

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

Replace Pin.depth with Pin.mode string enum #16

Closed
wants to merge 1 commit into from

Conversation

lidel
Copy link
Member

@lidel lidel commented Jul 10, 2020

See also for alternative simpler boolean version.

This PR removes ambiguity of depth: -1 for recursive pins (cc @ achingbrain) and enables arbitrary modes of pinning to be added in the future.

I included the third type named 'pinset', which could represent a well-known format for storing all local pins (cc @aschmahmann, we discussed something like this last week), but we can remove it if its not feasible atm.

cc @obo20

This enables arbitrary modes of pinning to be added in the future.
Third type name 'pinset' could represent object storing all local pins.
format: int32
default: -1
minimum: -1
mode:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoided type because its often a restricted keyword

@achingbrain
Copy link
Member

achingbrain commented Jul 10, 2020

I think depth?=n is better than mode=direct|recursive, I just didn't want depth: -1 because it's nonsensical.

depth also enables partial pinning which allows sharding massive datasets.

pinset as an enum value doesn't seem right to me either - if we need some way of grouping pins (e.g. all local pins as above), let's have multiple pinsets, and give them names.

@obo20
Copy link

obo20 commented Jul 10, 2020

Similarly to how pinning services handle depth (they don't) I'm pretty sure all pinning services (including Pinata) assume every pin is recursive in nature and I'm not aware of any service that specifically allows for direct pins. This may be another filter that isn't needed.

lidel added a commit that referenced this pull request Jul 13, 2020
lidel added a commit that referenced this pull request Jul 13, 2020
@lidel
Copy link
Member Author

lidel commented Jul 13, 2020

Thanks! Sounds like we can close this in favor of (optional) #19

@lidel lidel closed this Jul 13, 2020
@lidel lidel deleted the refactor/pin-types branch July 13, 2020 12:10
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.

3 participants