[openstreetmap/openstreetmap-website] Add limit parameter to api changesets query (PR #3933)
Andy Allan
notifications at github.com
Wed Jul 26 15:51:21 UTC 2023
Thanks, looks good to me too. Merged.
If you have a chance for a followup PR, there's some refactoring that could help here (and also for notes):
* Move the magic numbers to settings.yml, so that they are in the same place as other api paging stuff like tracepoints_per_page
* Expose them in the capabilities document, since that allows API consumers to avoid hard-coding matching magic numbers into their own code
* Refactor the tests to use the values, e.g. instead of checking for `get changesets_path(:limit => "101")` check for `get changesets_path(:limit => Settings.foo + 1)`
* When testing limits, I like to make sure we have a pair of tests for highest-permitted and lowest-forbidden. This avoids off-by-one errors. So an additional test for `:limit => Settings.foo`. You can see that e.g. in the tests where we check "255" works and "256" fails.
* Add similar tests for notes, I couldn't find any tests for these limits there.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3933#issuecomment-1652088815
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/3933/c1652088815 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230726/11850777/attachment-0001.htm>
More information about the rails-dev
mailing list