Page MenuHomeSoftware Heritage

swh.indexer.storage: Open content_mimetype_get_range
ClosedPublic

Authored by ardumont on Nov 14 2018, 12:32 PM.

Diff Detail

Repository
rDCIDX Metadata indexer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

  • Makefile: Run all tests by default
  • d/control: Fix space instead of tab
ardumont marked an inline comment as done.
  • tests: Remove uneeded code
ardumont added inline comments.
swh/indexer/storage/db.py
117

This is not used yet.
It shows how i plan to use it when adding new contents range endpoints.

swh/indexer/tests/storage/test_storage.py
1632 ↗(On Diff #2038)

No need for this, will remove.

I don't like the name content_types here. It sounds like HTTP content-type, which is a mimetype. And it's syntactically very close to content_mimetypes whereas it has a completely different meaning.

I don't have a good idea to replace it, though. Maybe content_indexer_names, to highlight the relation with the indexer that generated that particular data?

.gitignore
14 ↗(On Diff #2038)

I don't know much about Hypothesis, but it looks like it uses this directory to store examples that used to trigger failures; so it's a useful dir. The doc recommends to only ignore .hypothesis/eval_source. https://hypothesis.readthedocs.io/en/3.1.0/database.html

Makefile.local
1 ↗(On Diff #2038)

Why --disable-warnings?

swh/indexer/storage/__init__.py
136

Optional[bytes] means it's either None or an instance of bytes. You should probably use Optional[Tuple[bytes, bytes]].

143

Should be content_types, not content_tables. Also, that means this line is not tested.

150

Why the +1?

184

Same comment as on _content_get_range's doc.

swh/indexer/tests/storage/__init__.py
81 ↗(On Diff #2040)

Should be named content_mimetypes

swh/indexer/tests/storage/test_storage.py
1651 ↗(On Diff #2040)

Should be named actual_ids.

1679 ↗(On Diff #2040)

Should be named actual_ids.

1695 ↗(On Diff #2040)

Redundant with self.assertEqual(expected_mimetypes2, actual_mimetypes2)

This revision now requires changes to proceed.Nov 14 2018, 2:16 PM
ardumont added inline comments.
.gitignore
14 ↗(On Diff #2038)

Well, thanks for raising concerns.

but:

$ tree .hypothesis | tail -1
10 directories, 321 files

I don't want to commit that much data ;)

Makefile.local
1 ↗(On Diff #2038)

because i forgot about it ;)

swh/indexer/storage/__init__.py
136

next is either None or an instance of bytes.

143

Nice catch.

swh/indexer/tests/storage/test_storage.py
1695 ↗(On Diff #2040)

Yes, it's remains from my incremental way of testing.

ardumont added inline comments.
swh/indexer/storage/__init__.py
150

To take the last one as next (if any).

ardumont added inline comments.
swh/indexer/storage/__init__.py
143

fyi, that conditional is not tested... because i don't know how to test due to the db thing...
The db thing is installed by the decorators @db_transaction* but there ain't any here since it's an helper function.

ardumont added inline comments.
swh/indexer/storage/__init__.py
184

same answer, next is an Optional[bytes].

Fixes according to review:

  • Makefile: Do not activate the --disable-warnings flag
  • tests: Rename variable to content_mimetypes
  • indexer.storage: Fix wrong variable use
  • tests: Fix variables names
  • storage: Rename content_types to content_indexer_names
vlorentz added inline comments.
swh/indexer/storage/__init__.py
136

Oh sorry, I misread the description. I assumed from "The next range" that it was a range -_-

This revision is now accepted and ready to land.Nov 14 2018, 3:21 PM
  • tests: Use sorted directly on lists
This revision was automatically updated to reflect the committed changes.