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

Update ratelimit headers #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update ratelimit headers #29

wants to merge 2 commits into from

Conversation

DeeJayTC
Copy link

Changing the ratelimit headers to what we've been using already to make it consistent with old and new api. No need to change what is well known and working.

x-rateLimit is a common term used by many publishers.
see https://developer.github.com/v3/rate_limit/ as an example

splitting the term ratelimit in two parts is just unnecessary.

change headers to whats used already
.
.
@ripexz
Copy link
Member

ripexz commented Aug 29, 2018

Won't it break anything that already uses these headers though? 🤔

Also if we're making a backwards-incompatible change, we should probably drop the X- prefix - as of RFC6648 they're deprecated:

  1. Recommendations for Creators of New Parameters
    ...
    SHOULD NOT prefix their parameter names with "X-" or similar constructs.

@arp242
Copy link
Contributor

arp242 commented Aug 29, 2018

It was copied from Twitter IIRC, which does use X-Rate-Limit-...

I don't really care btw; don't really see the point in changing, but if you change it you should also update the API spec. I also don't know if we document this in any user-facing documentation (we might in Desk for example), so you would need to check that, too (but again, doesn't really seem worth it, IMHO).

@DeeJayTC
Copy link
Author

Yes it might break things for people who use theese headers atm but thats a lot less than people using the old headers. I've seen that rfc thing but its still commonly used across the board.

@Carpetsmoker the point is to be consistent with what we did before as theres, as you also say, no need to change theese headers. A lot more customers are used to the old x-ratelimit-header.

Someone needs to update his stuff for sure its just a matter of how many people.

Projects is overwriting those headers atm which shouldn't be needed ultimately if we have the same header everywhere.

@arp242
Copy link
Contributor

arp242 commented Aug 29, 2018

These were always the headers that were used in e.g. Desk, Launchpad, etc, so from that perspective there are no "new" and "old" headers.

@DeeJayTC
Copy link
Author

Yea just leave it open for now.

@arp242
Copy link
Contributor

arp242 commented Aug 29, 2018

To be clear, I'm not saying you can't change it, just that you need to be careful to change it everywhere, including the docs etc.

@DeeJayTC
Copy link
Author

yep absolutely right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants