Add python support #6
Conversation
There was a problem hiding this comment.
Looks good to me. The only comments that I have are general to the design of the library regardless of language, but nothing blocks landing here for sure.
The only thing I'm not sure on is the parameterize stuff in the unit tests. It looks reasonable but I'm not familiar with it.
| @@ -0,0 +1,27 @@ | |||
| import json | |||
There was a problem hiding this comment.
More a general point, maybe the best thing is to have one repository that has per-language directories where each langauge's project lives.
e.g.
jhford-work:lib-url jhford$ tree
.
├── go
│ └── urls.go
├── javascript
│ ├── index.js
│ ├── package.json
│ └── test.js
├── python
│ ├── module.py
│ └── setup.py
└── test-files
└── spec.yml
4 directories, 7 files
This would make it a little more clear what's going on, but still let us have commit-atomicity as well as a shared test spec. For extra points, we could even have a single test script in the root of the repository that will run the unit tests for all languages.
There was a problem hiding this comment.
I kind of wish we had done this in json-e, too -- the top-level of that repo is pretty crowded, and it's only by luck that various languages have not collided with one another (src for JS, jsone for Python, and jsone.go for Go)
There was a problem hiding this comment.
Yeah, I agree. I'm going to land all of this now to get it off my plate but we can circle back in a bit and make it look like what you specified up there ^
I admit I just cargo-culted what we did in jsone
| else: | ||
| return '{}/references/{}/{}/api.json'.format(root_url, service, version) | ||
|
|
||
| def docs(root_url, path): |
There was a problem hiding this comment.
more generally, should we have a version and service parameter in here, or a serviceDocs function? We shouldn't need to know how docs are layed out to get a URL to the docs for the specified service i suspect.
There was a problem hiding this comment.
It's for linking to manual and tutorial pages a well as reference pages. We want docs URLs to be stable over time (since people will bookmark them), so I think this is OK. Historically, the thing that's changed most with reference pages is the tier, and that would need to be supplied as a literal here either way.
| else: | ||
| return '{}/schemas/{}/{}'.format(root_url, service, name) | ||
|
|
||
| def ui(root_url, path): |
There was a problem hiding this comment.
Should we consider having the UI component name for a service in the api reference and then using just service name and version here as well? This library probably isn't the best place to encode the mapping between API and UI component, I suspect
There was a problem hiding this comment.
I think @eliperelman is getting away from the one-to-one mapping from UI-component to service. This method is just ${rootUrl}/${path} for everything but taskcluster.net, so it's really only useful for managing that special case (but nice-to-have for consistency anyway).
This is based on changes from #5, so we should land that one first if we land this one.