add *_PART_SIZE consts to public interface
#303
Conversation
| @@ -12,6 +12,8 @@ | |||
| # These constants are needed in different modules, so they are stored in this module, that | |||
| # imports nothing, thus avoiding circular imports | |||
|
|
|||
| LIST_FILE_NAMES_MAX_LIMIT = 10000 # https://www.backblaze.com/b2/docs/b2_list_file_names.html | |||
There was a problem hiding this comment.
But why is this exposed at all? It should be just some internal limitation of b2 and user should just iterate over pages seamlessly. And it should not be used by ourselves as well since there is always nextFileName field if there is next page.
There was a problem hiding this comment.
tl;dr: because we use this stuff in CLI
and longer answer:
what you wrote is exactly why. Because only nice interface for ls makes it so "seamless" that if we want to limit requests made, we need this number to be known in CLI
https://github.com/Backblaze/B2_Command_Line_Tool/blob/a478fb0e55c9274fdb2911f26b2006c00e890e66/b2/_cli/argcompleters.py#L35C1-L52
There was a problem hiding this comment.
I would separate list_files(page: int) (one page only) and iter_files() (just an iteration over pages, yielding from each page). I.e. I would move content of Bucket.ls's while True loop into a separate "per-page" method. May be an overkill though.
However, now that I looked at the Bucket class in sdk, it already uses that 10k limit (fetch_count: int | None = 10000), so I would rather make a constant there and replace fetch_count = 10000 with fetch_count = LIST_FILE_NAMES_MAX_LIMIT, and I would then import that constant in CLI as well. There should be one source of truth. Wdyt?
There was a problem hiding this comment.
I will use const in there as you suggested it is cheap and just makes sense.
IDK if adding manual pagination is worth it; we already gave a lot of manual control there to the user through fetch_count, I wouldn't complicate API more by adding another param for use case that can be already achieved thru what we have already.
test/unit/conftest.py
Outdated
| @pytest.fixture(scope='session') | ||
| def apiver_deps(): | ||
| import apiver_deps # noqa: F401 | ||
| return apiver_deps |
There was a problem hiding this comment.
this is more of preparation for this: https://github.com/reef-technologies/b2-sdk-python/blob/master/noxfile.py#L110
base on that comment long term it seems we want to have different apiver tests in the same pytest session.
Also this fixture nicely "contains" the import hack into the file that does this magical injecting ver dependent apiver_deps into sys.path .
Right now you have import apiver_deps all over the tests, which confuses both new devs and pycharm.
There was a problem hiding this comment.
Hm, I read that comment as "we want to have different nox sessions for each apiver test". Smth like:
@nox.parametrize('apiver', [0, 1, 2, 3])https://nox.thea.codes/en/stable/config.html#parametrizing-sessions
Ok, whatever, if you introduce the fixture then maybe replace everything with a fixture? It would be weird if one test uses the fixture and others don't
There was a problem hiding this comment.
you are right, I misread that, it must have been about nox.parametrize and not pytest
|
|
||
| @pytest.mark.apiver(from_ver=2) | ||
| def test_public_constants(apiver_deps): | ||
| assert set(dir(apiver_deps)) >= { |
mjurbanski-reef
force-pushed
the
public_consts
branch
from
69e3d41
to
27b153c
Compare
mjurbanski-reef
force-pushed
the
public_consts
branch
from
27b153c
to
bc0fbe3
Compare
No description provided.