ssbc / ssb-ref

check if a string is a valid ssb-reference
MIT License
14 stars 10 forks source link

Remove length limit from hashtag and consistent return #33

Closed christianbundy closed 3 years ago

christianbundy commented 5 years ago

before

after

christianbundy commented 5 years ago

cc @mmckegg, I just saw that you added the maximum length of 30 in https://github.com/ssbc/ssb-ref/pull/12#issue-151296358 -- what do you think about this?

dominictarr commented 4 years ago

The ui would generally need a limit. Also database indexes may need a limit (if they store the tag, but maybe not if its a hash table)

On Fri, Aug 23, 2019, 04:30 Will notifications@github.com wrote:

@qubist approved this pull request.

This seems fine to me, but I'm not sure why the limit was added in the first place: there might be a good reason there. Twitter has a maximum hashtag character count: [image: Screen Shot 2019-08-22 at 10 25 15 PM] https://user-images.githubusercontent.com/3607279/63562607-3d593480-c52c-11e9-8281-3d0fd1f67da0.png It caps hashtags at 100 characters from my experimenting. That seems reasonable and like no one would reasonable run into that. If there's a good reason to keep a limit, I suggest we change it to 100 characters.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ssbc/ssb-ref/pull/33?email_source=notifications&email_token=AAB7KLQLW3HBGPDPURI7J2TQF5DSZA5CNFSM4IOWDDTKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCO3NPQ#pullrequestreview-278771390, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB7KLT4TYHTXU65VRMJTGLQF5DSZANCNFSM4IOWDDTA .

christianbundy commented 4 years ago

@dominictarr Are you proposing that I change this branch to set a new limit, or just noting that there are limits in general outside of this module? Just realized I left this open because I wasn't sure how to read your comment.

dominictarr commented 4 years ago

well, if in doubt, there should probably be a limit. that means you can't have comedically or maliciously long hashtags... is that a great loss? have the been problems with things hitting the limit? it's better to keep limits the same than to change them.

I mean, we didn't put the limit there for fun it was for a reason. so we are gonna remove it we should have a good reason

christianbundy commented 4 years ago

How would you feel about a limit like 128? Seems to be big enough that nobody would hit it on accident, and if/when it causes bugs it's easy to shrug them off.

dominictarr commented 4 years ago

nothing wrong with 128 except the limit is currently 30 and you havn't made a case that there is anything wrong with 30 except you don't seem to like that number.

dominictarr commented 4 years ago

also, a hash tag is ment to be a easy to remember short tag... 30 characters is already too long, really

christianbundy commented 4 years ago

you havn't made a case that there is anything wrong with 30 except you don't seem to like that number.

also, a hash tag is ment to be a easy to remember short tag... 30 characters is already too long, really

Okay. I'm confused that easy to remember is an opinion that's baked into normalizeChannel(), which the documentation indicates "removes punctuation to make a standard channel name". Using this method causes issues in Patchwork -- I think clicking long hashtags took you to #undefined? -- and I wouldn't expect this function to enforce any arbitrary length.

Maybe you could help me understand where you're coming from a bit more? I'm not saying it's unreasonable to enforce a maximum hashtag length somewhere, but it doesn't seem appropriate here IMO. Maybe I'm missing context?

christianbundy commented 4 years ago

At the very least, maybe we could return data.slice(0, 30)? We'll still get collisions between:

But at least we aren't throwing everyone in the undefined pile and ignoring the input string.

dominictarr commented 4 years ago

oh yeah, truncating is definitely what should happen! then you'd get a channel #the-best-flavor-cake-flavor-is which is fine.

arj03 commented 4 years ago

The 30 characters seems to be in other places as well.

arj03 commented 3 years ago

Looking at old PRs and wanted to move this along. As I read this, it has converged to the only change being: invalid hashtags will intentionally return null. That will help patchwork with long tags. Is that corrected understood @christianbundy? And if so, can you update the PR.

christianbundy commented 3 years ago

Superseded by #43