Page MenuHomeSoftware Heritage

Fixes for many past and present tasks

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

Diff Detail

rDWAPPS Web applications
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

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

221 ↗(On Diff #317)

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

196–201 ↗(On Diff #317)

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).


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

  • 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

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.
221 ↗(On Diff #317)

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

223 ↗(On Diff #317)

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


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.