Add types and allow empty app ID #41
Conversation
|
This change makes sense and is simple enough, but you've got a couple of missing double quotes in your type JSON tags. |
|
I'm not sure if I should be more scared that Go never raised an error or that none of my tests picked it up |
| Status struct { | ||
| Code string `json:"code"` | ||
| } `json:"status"` | ||
| Values *[]ResolutionValue `json:"values"` |
There was a problem hiding this comment.
Why make this a pointer to a slice? Under the hood a slice uses a pointer.
| Values *[]ResolutionValue `json:"values"` | ||
| } | ||
|
|
||
| type ResolutionValue struct { |
There was a problem hiding this comment.
Since we are prefixing other types with Echo perhaps this should be EchoResolutionValue?
| @@ -159,8 +159,9 @@ func verifyJSON(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) { | |||
| return | |||
| } | |||
|
|
|||
| // Check the app id | |||
| if !echoReq.VerifyAppID(Applications[r.URL.Path].(EchoApplication).AppID) { | |||
| if Applications[r.URL.Path] != nil && | |||
There was a problem hiding this comment.
I wonder if we should add this nil check logic to VerifyAppID? It would then be much cleaner:
if !echoReq.VerifyAppID() {
HTTPError(...)
}
|
@warent Some IDEs (I use GoLand) will call out/highlight malformed tags. |
|
The |
|
What's the status on this PR? This conflicts with the current master but if it's still needed, I can merge. (Also, please don't hesitate to ping me on Twitter [@thatmikeflynn] as my GitHub notifications are impossible to keep up with and for some reason this repo is always at the bottom of the list.) |
|
@mikeflynn given what @rking788 mentioned how difficult would it be to add a CI check (with |
|
That would be great! Have either of you set that up with GitHub PRs before? Any way you can point me in the right direction? |
|
so I'm not sure if there is a different way that I have never tried before. but the only way I can think of is to setup a travis.ci build for the repo (free for open source projects) and then run |
|
I can try to setup something on my own fork of this project and then, once I get it working, I'll ping y'all. |
|
@rking788 @mikeflynn PR for adding Travis CI (with instructions!): #45 |
|
Looks like this can now be rebased and then merged? |
|
So this got kind of lost in the shuffle. Still important? If someone can resolve the conflict we can merge. |
No description provided.