in turns, this allow to use mmap() random writing when restoring int->pid maps
Details
- Reviewers
seirl - Group Reviewers
Reviewers - Commits
- rDGRPH636278c6fc43: pid.py: use a dict for more idiomatic file mode check
rDGRPHdaff920072ed: cli.py: avoid importing unused PID_BIN_SIZE constant
rDGRPH94c6e1ed7457: test_pid.py: fix alphabetic ordering of node types
rDGRPHf06713d216c7: pid.py: avoid importing unused mmap constants
rDGRPH66cfb3625025: CLI: make restore of int->pid maps use mmap writing instead of seek
rDGRPH84f223a0eeb3: pid maps: add limited support for updatable maps
pytest
Diff Detail
- Repository
- rDGRPH Compressed graph representation
- Branch
- feature/writable-maps
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 7727 Build 11104: tox-on-jenkins Jenkins Build 11103: arc lint + arc unit
Event Timeline
Build has FAILED
Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/40/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/40/console
Build has FAILED
Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/41/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/41/console
Build is green
See https://jenkins.softwareheritage.org/job/DGRPH/job/tox/42/ for more details.
Looks globally ok to me, so feel free to ignore my comments.
swh/graph/cli.py | ||
---|---|---|
95–100 | This is not part of the diff, but I tend to find weird to use required options in a cli tool... | |
swh/graph/pid.py | ||
101–107 | I tend to prefer a dict-based approach for this kind of stuff like (not properly formatted): MODES = {'rb': os.O_RDONLY, 'wb': os.O_RDWR | os.O_CREAT, 'rb+': os.O_RDWR} os_mode = MODES.get(mode) |
swh/graph/tests/test_pid.py | ||
---|---|---|
54 | Why this change? |
swh/graph/cli.py | ||
---|---|---|
95–100 | That's a fair point. But when you start having multiple positional argument, it's a pain to remember which one goes where if there isn't an obvious order (e.g., cp's src/dst). So I'd rather keep it like this for now. | |
swh/graph/tests/test_pid.py | ||
54 | Readability. The order here doesn't matter, as there is a sorted in the function implementation anyway. But it was a pain to read as a default argument in a non-alphabetic order. |
swh/graph/pid.py | ||
---|---|---|
101–107 | (This also allows you to check if mode not in MODES at the beginning. |
- test_pid.py: fix alphabetic ordering of node types
swh/graph/tests/test_pid.py | ||
---|---|---|
54 | fixed now |
Build is green
See https://jenkins.softwareheritage.org/job/DGRPH/job/tox/43/ for more details.
swh/graph/pid.py | ||
---|---|---|
101–107 | I love these two together! Fixed |
Build is green
See https://jenkins.softwareheritage.org/job/DGRPH/job/tox/44/ for more details.
- pid maps: add limited support for updatable maps
- CLI: make restore of int->pid maps use mmap writing instead of seek
- pid.py: avoid importing unusued mmap constants
- cli.py: avoid importing unused PID_BIN_SIZE constant
- test_pid.py: fix alphabetic ordering of node types
- pid.py: use a dict for more idiomatic file mode check
Build is green
See https://jenkins.softwareheritage.org/job/DGRPH/job/tox/46/ for more details.
- pid.py: avoid importing unused mmap constants
- cli.py: avoid importing unused PID_BIN_SIZE constant
- test_pid.py: fix alphabetic ordering of node types
- pid.py: use a dict for more idiomatic file mode check
Build is green
See https://jenkins.softwareheritage.org/job/DGRPH/job/tox/47/ for more details.