Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the new /api/links endpoint #4524

Merged
merged 1 commit into from
Apr 13, 2017
Merged

Add the new /api/links endpoint #4524

merged 1 commit into from
Apr 13, 2017

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Apr 12, 2017

Add the new /api/links endpoint and add a link to this endpoint to the existing /api endpoint. This new endpoint will be used by the client to get the URL templates for generating URLs to HTML pages of the web service. This is part of hypothesis/product-backlog#215

@seanh seanh force-pushed the add-links-api-2 branch from 88f6dd2 to 3b34c6e Compare April 12, 2017 15:32
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two suggestions:

  1. That we should make the endpoint self-documenting and consistent with /api
  2. That we should indicate somehow that groups.leave is deprecated

user_url = request.route_url('stream.user_query', user='_user_')
user_url = user_url.replace('_user_', ':user')

return {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice if this endpoint was self-documenting in a similar way to the /api endpoint. We could do this by keeping the key as the route name but changing the value to a { url, desc } object, consistent with the /api endpoint. Thoughts?

Copy link
Contributor Author

@seanh seanh Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually discussed this on Slack way back (one of the Slack convo's that the product-backlog issue for this links to, I think) because I wanted each link to be an object with url and method, but the method would always be GET, and these links aren't API endpoints like the /api ones are they're just links to be rendered as hrefs and the browser directed to, so we decided on a simple list of name -> URL. It's the same as what the github API uses.

What would be the benefit of adding descs to these, beyond the already-existing descriptiveness of the name and the URL itself?

account.settings: {
  url: "http://localhost:5000/account/settings",
  desc: "The account settings page"
}

I'm not sure it adds much.

I suppose we could add, in the desc or in an additional key, that groups.leave is deprecated. But I'm not sure anyone would notice it. A developer programs their client to use the links API once, later on deprecated is added to the desc of one of the links, but unless the developer is manually looking at the /api/links output again after having already finished programming their client to use it, they aren't likely to notice are they? (Of course developers writing new clients might see it...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanh - OK, if we've already had the discussion I don't have a strong enough opinion to justify rehashing it. Let's go with this as-is.

return {
'account.settings': request.route_url('account'),
'forgot-password': request.route_url('forgot_password'),
'groups.leave': group_leave_url,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we follow the suggestion above to add a description to each link, I would then explicitly call out the fact that groups.leave is deprecated.

@codecov-io
Copy link

codecov-io commented Apr 12, 2017

Codecov Report

Merging #4524 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4524      +/-   ##
==========================================
+ Coverage   94.89%   94.89%   +<.01%     
==========================================
  Files         352      352              
  Lines       18965    18988      +23     
  Branches     1102     1102              
==========================================
+ Hits        17996    18019      +23     
+ Misses        868      865       -3     
- Partials      101      104       +3
Impacted Files Coverage Δ
tests/h/routes_test.py 100% <ø> (ø) ⬆️
h/routes.py 100% <100%> (ø) ⬆️
h/views/api.py 100% <100%> (ø) ⬆️
tests/h/views/api_test.py 99.34% <100%> (+0.03%) ⬆️
h/stats.py 83.33% <0%> (ø) ⬆️
h/search/__init__.py 38.88% <0%> (ø) ⬆️
h/session.py 91.83% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53046f6...3b34c6e. Read the comment docs.

@seanh
Copy link
Contributor Author

seanh commented Apr 12, 2017

I've added the new feature flag to this but note that there's an issue over in the client PR with the client being able to actually use this feature flag

@seanh seanh force-pushed the add-links-api-2 branch from b4c81df to 3b34c6e Compare April 13, 2017 12:52
@seanh seanh merged commit 5504854 into master Apr 13, 2017
@seanh seanh deleted the add-links-api-2 branch April 13, 2017 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants