add eskip json unmarshalling #471
Conversation
|
Looks good (for my limited understanding of Go). As for the tests, it might not be very idiomatic, but it might be nice to have some kind generative tests, that ensure: |
|
ok, thanks, will add such tests |
|
@aryszka is this still something we want to have, if so I can pick it up probably, otherwise lets close the PR. |
|
@grassator yes, we need this 😄 |
|
@aryszka I'm picking this up then. |
grassator
force-pushed
the
feature/eskip-from-json
branch
from
6d66bca to
01d8b85
Compare
|
@aryszka I've rebased on top of master and had to make a few changes on the go. Most of them are still keeping API compatible, the only really breaking change if someone was creating |
eskip/json.go
Outdated
| func (p *Predicate) MarshalJSON() ([]byte, error) { | ||
| return marshalNameArgs(p.Name, p.Args) | ||
| } | ||
|
|
||
| // UnmarshalJSON returns the JSON representation of a predicate. |
There was a problem hiding this comment.
Are you sure that you don’t do it the other way around?
Marshal and unmarshal have very similar doc strings.
There was a problem hiding this comment.
I agree that the doc comment should rather say something like this:
"UnmarshalJSON parses a predicate object from its JSON representation."
eskip/json.go
Outdated
| }{ | ||
| var buf bytes.Buffer | ||
| e := json.NewEncoder(&buf) | ||
| e.SetEscapeHTML(false) // why not working :( |
There was a problem hiding this comment.
There was a problem hiding this comment.
That was your comment :) I will check
There was a problem hiding this comment.
So this took a bit to figure out. The problem is that the setting only has effect if you are using an encoder directly. If you use json.Marshal there is no way to unset it.
https://golang.org/src/encoding/json/encode.go?s=6521:6546#L160
eskipfile/watch.go
Outdated
| return WatchFile(fileName, eskip.ParseBytes) | ||
| } | ||
|
|
||
| func WatchFile(fileName string, parseFunc eskip.ParseFunc) *WatchClient { |
There was a problem hiding this comment.
the name of this function should reflect what's the difference from the simple Watch(). My candidate is: WatchParser()
Should better reflect the difference from original `Watch` function
| @@ -334,6 +336,11 @@ func Parse(code string) ([]*Route, error) { | |||
| return routeDefinitions, nil | |||
| } | |||
|
|
|||
| // Parses a route expression or a routing document to a set of route definitions. | |||
There was a problem hiding this comment.
According the https://golang.org/doc/effective_go.html#commentary comments should start with function name:
| // Parses a route expression or a routing document to a set of route definitions. | |
| // ParseBytes parses a route expression or a routing document to a set of route definitions. |
It some languages, to generate docs, function name is ignored, that's why doc comments starts with function name.
Fixes #879