White/Black rank converter #208
Conversation
Brandf3
requested review from
a team,
artasparks and
TrevorPeters
and removed request for
a team
core/prop/ranks.go
Outdated
| Scope: RootScope, | ||
| From: func(n *movetree.Node, prop string, data []string) error { | ||
| if len(data) != 1 { | ||
| return fmt.Errorf("%s propertie requires exactly 1 value, but had %d", prop, len(data)) |
core/prop/ranks.go
Outdated
| }, | ||
| } | ||
|
|
||
| func isValid(rank string) error { |
There was a problem hiding this comment.
I would rename this as isValidRank, since this exists in the package-namespace. So, if another file had isValid, it would confilct
| @@ -0,0 +1,220 @@ | |||
| package prop | |||
core/movetree/node.go
Outdated
| @@ -17,6 +17,10 @@ type GameInfo struct { | |||
|
|
|||
| // Initial player turn. This is traditionally the player with the black stones | |||
| Player color.Color | |||
|
|
|||
| // Ranks of both players. Between 1-30k/kyu, 1-9d/dan, or 1-9p/pro | |||
| WhiteRank string | |||
There was a problem hiding this comment.
Comments should have the format
"WhiteRank is the rank of the white player."
"blackRank is the rank of the black player."
Try running golint ./...
| if err != nil { | ||
| return "", err | ||
| } | ||
| out.WriteString("BR[" + rank + "]") |
There was a problem hiding this comment.
nit: string builder is a bit of overkill if you're only constructing the string once.
|
|
||
| func isValidRank(rank string) error { | ||
| // Check that rank ends with a valid string | ||
| r, _ := regexp.Compile("(k|kyu|d|dan|p|pro)\\b") |
There was a problem hiding this comment.
I would recommend putting this outside the method:
var rankRegexp = regexp.MustCompile(...)
I wonder whether this is overkill though. Well, we can profile later.
| }, | ||
| } | ||
|
|
||
| func isValidRank(rank string) error { |
There was a problem hiding this comment.
I just re-read over the spec. https://www.red-bean.com/sgf/properties.html#BR
BR and WR are just simpletext. There are conventions, but I don't think we can guarantee the format. So, rather than being restrictive, I think we should probably remove the intensive validation and just do the basic validation.
Fixes #182