search

marvinody / mercari

a wrapper around mercari jp shopping site
43 stars 15 forks source link

Param `limit` should be the variable #16

Closed kbdancer closed 1 year ago

kbdancer commented 1 year ago

https://github.com/marvinody/mercari/blob/359bdf705aa3046015bbea517a31bcd8fa14935c/mercari/mercari.py#L82

marvinody commented 1 year ago

I think removing it altogether would make more sense. The code will iterate through the pages until the API gives no more results which is how my projects end up using it instead of a global "stop after this many search results." It doesn't affect how many results are returned from the library, just how many chunks to go out to Mercari for.

Allowing a user to pass a lower limit would result in more API requests being sent on average and probably a net loss for the user.

kbdancer commented 1 year ago

I can understand what you mean. In the current usage scenario (to get all the results), this parameter is really not needed. I think removing def search(keywords, sort="created_time", order="desc", status="on_sale" , limit=120) on limit=120 will be more friendly to developers who use this library for the first time, because I only use it to get the first few dozen pieces of data, but when setting limit=30 After that, it didn't work, so I have to make some changes based on your code so that I can achieve the required functions, such as limit can be controlled, and the traversal of the number of pages can be removed. Of course, this is just my personal suggestion, and thank you for your reply.

marvinody commented 1 year ago

If you wanted to make a PR that introduces a new function, maybe search_paginated or paginated_search, that accepts a page parameter and a limit parameter (that actually gets used) which returns data as you expect, I'd be happy to merge and publish!