4 ms·

A couple of suggestions: Lock down the prefix string now before it’s too late and document it. I see in Go that it’s lowercase ascii, which seems fine except f

by klabb3 3y ago

A couple of suggestions: Lock down the prefix string now before it’s too late and document it. I see in Go that it’s lowercase ascii, which seems fine except for compound types (like “article-comment”). May be worth looking at allowing a single separator given that many complex projects (and ORMs) can’t avoid them. The Go implementation has no tests. This is very unit-testable. Add tests goddammit! For Go, I’d align with Googles UUID implementation, with proper parse functions and an internal byte array instead of strings. Strings are for rendering (and in your case, the prefix). Right now, it looks like the parsing is too permissive, and goes into generation mode if the suffix is empty. And the SplitN+index thing will panic if no underscores, no? Anyway, tests will tell. As for the actual design decisions, I tried to poke holes but I fold! I think this strikes the sweet spot between the different tradeoffs. Well done!

avgcorrection 3y ago
> The Go implementation has no tests. This is very unit-testable. Add tests goddammit! Yep. The readme asks people to provide other implementations. Having a test suite would be good for third-party code.
tomcam 3y ago
> Add tests goddammit! Hey, you’re pretty smart. How about you add them?
- klabb3 3y ago
  I’m by no means a test police. I’m in fact opposed to a lot of mindless testing for the sake of it. But there are places where unit tests shine, and this is one of them. If you mean that criticism is only allowed if you are willing to commit labor, I disagree with that. I always welcome critique myself - it may be something that I’ve missed. The maintainers always has the last word. As long as there are no hidden expectations, it’s all good.
  - tomcam 3y ago
    Do you need to be “test police“ to add tests you literally demanded of them?
dloreto 3y ago
Thanks for the feedback! We have tests for the base32 encoding which is the most complicated part of the implementation (https://github.com/jetpack-io/typeid-go/blob/main/base32/base32_test.go https://github.com/jetpack-io/typeid-go/blob/main/base32/bas...) but your point stands. We'll add a more rigorous test suite (particularly as the number of implementations across different languages grows, and we want to make sure all the implementations are compatible with each other) Re: prefix, is the concern that I haven't defined the allowed character set as part of the spec?
- klabb3 3y ago
  > We have tests for the base32 encoding which is the most complicated part of the implementation I didn’t look into it much but it seems like a great encoding even outside of this project. Predictable length, reasonable density, “double clickable” etc. I’ve been annoyed with both hex and base64 for a while so it’s pretty cool just by itself. > Re: prefix, is the concern that I haven't defined the allowed character set as part of the spec? Yeah, the worry is almost entirely “subtle deviations across stacks”, which is usually due to ambiguous specs. It’s so annoying when there’s minor differences, compatibility options etc (like base64 which has another “URL-friendly” encoding - ugh).
  - aftbit 3y ago
    My personal favorite encoding is base58 aka Bitcoin address encoding. It uses characters [A-Za-z0-9] except for [0OIl]. It is almost as dense as base64, "double clickable", but not (as) predictable in length as base32. It was chosen to avoid a number of the most annoying ambiguous letter shapes for hand-entry of long address strings. https://en.bitcoin.it/wiki/Base58Check_encoding https://en.bitcoin.it/wiki/Base58Check_encoding
    - bredren 3y ago
      Reminds me that Windows activation keys used to exclude a broader set of characters to avoid transcription errors: looking it up again: 0OI1 and 5AELNSUZ
    - 3y ago
rtheunissen 3y ago
I'll write some tests for this.
jhoechtl 3y ago
I think your misconception is that prefix is sthg. fixed? You decided on the prefix depending on the usage domain.
dloreto 3y ago
A follow up: 1. We've now implemented pretty thorough testing: https://github.com/jetpack-io/typeid-go/blob/main/typeid_test.go https://github.com/jetpack-io/typeid-go/blob/main/typeid_tes... 2. I clarified the prefix in the spec Thanks for the feedback!

by klabb3 3y ago

avgcorrection 3y ago

> The Go implementation has no tests. This is very unit-testable. Add tests goddammit! Yep. The readme asks people to provide other implementations. Having a test suite would be good for third-party code.

tomcam 3y ago

> Add tests goddammit! Hey, you’re pretty smart. How about you add them?

klabb3 3y ago
I’m by no means a test police. I’m in fact opposed to a lot of mindless testing for the sake of it. But there are places where unit tests shine, and this is one of them. If you mean that criticism is only allowed if you are willing to commit labor, I disagree with that. I always welcome critique myself - it may be something that I’ve missed. The maintainers always has the last word. As long as there are no hidden expectations, it’s all good.
- tomcam 3y ago
  Do you need to be “test police“ to add tests you literally demanded of them?

dloreto 3y ago

Thanks for the feedback! We have tests for the base32 encoding which is the most complicated part of the implementation (https://github.com/jetpack-io/typeid-go/blob/main/base32/base32_test.go https://github.com/jetpack-io/typeid-go/blob/main/base32/bas...) but your point stands. We'll add a more rigorous test suite (particularly as the number of implementations across different languages grows, and we want to make sure all the implementations are compatible with each other) Re: prefix, is the concern that I haven't defined the allowed character set as part of the spec?

klabb3 3y ago
> We have tests for the base32 encoding which is the most complicated part of the implementation I didn’t look into it much but it seems like a great encoding even outside of this project. Predictable length, reasonable density, “double clickable” etc. I’ve been annoyed with both hex and base64 for a while so it’s pretty cool just by itself. > Re: prefix, is the concern that I haven't defined the allowed character set as part of the spec? Yeah, the worry is almost entirely “subtle deviations across stacks”, which is usually due to ambiguous specs. It’s so annoying when there’s minor differences, compatibility options etc (like base64 which has another “URL-friendly” encoding - ugh).
- aftbit 3y ago
  My personal favorite encoding is base58 aka Bitcoin address encoding. It uses characters [A-Za-z0-9] except for [0OIl]. It is almost as dense as base64, "double clickable", but not (as) predictable in length as base32. It was chosen to avoid a number of the most annoying ambiguous letter shapes for hand-entry of long address strings. https://en.bitcoin.it/wiki/Base58Check_encoding https://en.bitcoin.it/wiki/Base58Check_encoding
  - bredren 3y ago
    Reminds me that Windows activation keys used to exclude a broader set of characters to avoid transcription errors: looking it up again: 0OI1 and 5AELNSUZ
  - 3y ago

rtheunissen 3y ago

I'll write some tests for this.

jhoechtl 3y ago

I think your misconception is that prefix is sthg. fixed? You decided on the prefix depending on the usage domain.

dloreto 3y ago

A follow up: 1. We've now implemented pretty thorough testing: https://github.com/jetpack-io/typeid-go/blob/main/typeid_test.go https://github.com/jetpack-io/typeid-go/blob/main/typeid_tes... 2. I clarified the prefix in the spec Thanks for the feedback!