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

docs: case-insensitive Pin.name filter #58

Merged
merged 2 commits into from
Sep 10, 2020
Merged

docs: case-insensitive Pin.name filter #58

merged 2 commits into from
Sep 10, 2020

Conversation

lidel
Copy link
Member

@lidel lidel commented Sep 2, 2020

This PR updates description of name filter and removes ambiguity, as noted in #45 (comment) by @andrew and @obo20

The rationale for going with case-sensitive is that it is more performance out of the box, does not require any additional indexes or optimizations, and one can use third-party case-sensitive identifiers (other than CID).

Case-insensitive makes it easier to search and returns not false-negatives, but comes with a risk for bugs when a search results in table scan even when there is an index on the name column.

Let's review which way to go in this PR.

Update: we went with case-insensitive

This removes ambiguity, as noted in
#45 (comment)

The rationale for going with case-sensitive is that it is more
performance out of the box, does not require any additional indexes or
optimizations.

Case-insensitive comes with a risk for bugs when a search results in table scan
even when there is an index on the name column.
@lidel lidel requested a review from a team September 2, 2020 09:59
Copy link

@andrew andrew left a comment

Choose a reason for hiding this comment

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

👍

@jacobheun
Copy link
Collaborator

I get the performance aspect of this, but if I was implementing the API I'd want to and would violate this as the user experience is not great. The whole point of naming a pin is to create a human identifiable association to it. If I pinned a collection of photos from 2018 and called it "Photos-2018", because I'm human and name things inconsistently, and couldn't search for it with 'photos' that would be frustrating.

I'd much rather see the recommendation be for case-insensitive searching purely for usability reasons.

@lidel
Copy link
Member Author

lidel commented Sep 4, 2020

Both approaches have merit, I wonder what would be better:

  • making name case-insensitive (and making it useless for case-sensitive identifiers)
  • supporting both: case-insensitive name and add separate filter nameCaseSensitive – needs better... name)

Thoughts?

@jacobheun
Copy link
Collaborator

supporting both: case-insensitive name and add separate filter nameCaseSensitive – needs better... name)

I'm not seeing the value this provides that would justify the added complexity. You mentioned 3rd party identifiers being restricted to case-insensitive usage if we went case-insensitive only, but I don't see the harm in that or a good reason to use a identifier generator for pin names to begin with, did you have something in particular in mind?

As I see it, the point of a pin name is to create a meaningful label so that I will know what the content is for the given CID. As long as the name is meaningful to the end user, case sensitivity doesn't really matter during a query. You'd still be able to view the casing on the name when it's returned in the search results, if that has meaning to you, but finding the content by name becomes much easier.

@lidel lidel changed the title docs: case-sensitive Pin.name filter docs: case-insensitive Pin.name filter Sep 7, 2020
@lidel
Copy link
Member Author

lidel commented Sep 7, 2020

@obo20 @andrew @jacobheun I flipped it to be case-insensitive:

  • Agree its main value of name field is ease of finding pin by human-readable labels, and presenting them in UIs, so going with case-insensitive improves UX by removing the problem of false-negatives.

  • The case-sensitive identifiers I mentioned could be something that is specific to an app, but one could either re-encode them to case-insensitive form, or use Pin.meta[custom_id] instead.

Mind reviewing again?

@lidel lidel requested review from a team and andrew September 7, 2020 12:38
Copy link

@andrew andrew left a comment

Choose a reason for hiding this comment

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

👍

andrew added a commit to ipfs-shipyard/rb-pinning-service-api that referenced this pull request Sep 7, 2020
@lidel
Copy link
Member Author

lidel commented Sep 10, 2020

As this is only clarifying change to docs, I feel its safe to merge

@lidel lidel merged commit c9050a4 into master Sep 10, 2020
@lidel lidel deleted the docs/name-case branch September 10, 2020 14: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