3 ms·
Some Go API style nits: - it's unusual to pass a map with expected keys like this. Normally you'd pass a struct like: p := New(Options{ BaseColor: "...", C
by ImJasonH 12y ago
Some Go API style nits:
- it's unusual to pass a map with expected keys like this. Normally you'd pass a struct like:
p := New(Options{
BaseColor: "...",
Color: "...",
Generator: "...",
})
- You could define the known patterns as constants so they're documented and so users don't have to worry about typos
- Field names are normally camel-case without underscores, e.g., "Base_color" should be "BaseColor"
- You could use Go's build-in image/color package instead of accepting RGB strings, but that's more of a judgement call.
If you're accepting pull requests I might send you some, but they're breaking API changes so it's up to you.
All-in-all though it's a cool project and I'm glad to see it in Go :)
- pravj 12y agoHi @ImJasonH, I Developed this, let me answer you points :) I believe these points are not contributing in breaking API :D so lets talk on them first. 2. You mean instead of an array I'm using here [1], We should add them as constants and write separate documentation for each? This sounds nice as it will help obviously like you said. 3. Ohh, I missed this convention, thanks for pointing this out. :) 4. As you said its more of a judgement call, I believe that RGB strings are fine and easy for users. You find any wrong in RGB strings? So, I'll be very happy to co-operate if you are doing pull request covering points 2 and 3 :) And I'll try to cover your point 1 on my own :) Cheers \o/ [1] https://github.com/pravj/geo_pattern/blob/master/pattern/pattern.go#L17 https://github.com/pravj/geo_pattern/blob/master/pattern/pat...
- pdpi 12y agoRegarding 4, I personally tend to be distrustful of what is often described as Stringly Typed code. To me, statically constraining your API to only accept valid inputs seems like superior design.
- infogulch 12y agoIf you're reworking your API, you may want to consider functional options: http://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis http://dave.cheney.net/2014/10/17/functional-options-for-fri... I haven't attempted it myself, but it looks very interesting.
- betamike 12y agoAnother minor nitpick, here [1] you are using reflection. You can pretty easily convert it to a type switch [2], which is more idiomatic. [1] https://github.com/pravj/geo_pattern/blob/master/svg/svg.go#L81 https://github.com/pravj/geo_pattern/blob/master/svg/svg.go#... [2] https://golang.org/doc/effective_go.html#type_switch https://golang.org/doc/effective_go.html#type_switch