Page MenuHomeSoftware Heritage

Fixes for many past and present tasks
ClosedPublic

Authored by jbertran on Aug 10 2016, 11:52 AM.

Diff Detail

Repository
rDWAPPS Web applications
Branch
T431
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 368
Build 543: Software Heritage Python tests
Build 542: arc lint + arc unit

Event Timeline

jbertran retitled this revision from to Fixes for many past and present tasks.
jbertran updated this object.
jbertran edited the test plan for this revision. (Show Details)
jbertran added a reviewer: zack.
zack requested changes to this revision.Aug 12 2016, 10:46 AM
zack edited edge metadata.

Great clean up commit!
I've just requested some minor changes here and there

swh/web/ui/backend.py
222

it'd be more pythonic to test this as "if not rev_list"

swh/web/ui/converters.py
196–202

The del() here is bad: content is aliased here, so you will end up modifying in place the argument passed to from_content(), which could be very surprising for the caller. Please copy the dictionary instead (or slice the needed parts of it before returning it further, or equivalent).

swh/web/ui/service.py
297

This magic 25 value appears in too many places, I've counted at least 3.
Please:

  • either use a constant in the relevant module, and use the constant everywhere else
  • or use a "sandwich" architecture, in which the default value is set only in the outer functions, and make the inner ones have mandatory (rather than optional) arguments
swh/web/ui/tests/test_app.py
51

Default configuration values will be added to this test configuration, right? If so, you can probably avoid setting max_log_revs explicitly here.

This revision now requires changes to proceed.Aug 12 2016, 10:46 AM
olasd added inline comments.
swh/web/ui/backend.py
222

I think we want an empty list to pass through as an empty list, not as None.

224

This limiting should be done on the SQL side, not here.

jbertran edited edge metadata.
jbertran marked 6 inline comments as done.

Changes from diff feedback for D96, see commit msg

swh/web/ui/tests/test_app.py
51

I believed so, but found out it wasn't the case when running the tests. I'm not quite sure why though, since the next lines should use the conf of the actual app if I have it running at the same time, which I am.

zack edited edge metadata.
This revision is now accepted and ready to land.Aug 16 2016, 10:29 AM
This revision was automatically updated to reflect the committed changes.