Code review comment for ~racb/git-ubuntu:interleave-publication-dates

Revision history for this message
Bryce Harrington (bryce) wrote :

√ python3 setup.py build
√ python3 setup.py check
√ ./snap-wrappers/wrappers/git-ubuntu-self-test
  √ PASS (syntax) source-package-walker.py
  √ PASS (compilation) source-package-walker.py
  √ PASS (syntax) import-source-packages.py
  √ PASS (compilation) import-source-packages.py
  √ PASS (syntax) update-repository-alias.py
  √ PASS (compilation) update-repository-alias.py
  √ 335 passed, 3 xfailed in 183.72 seconds
  √ Coverage for importer.py at 64%
  √ Overall coverage 54%
√ Per-patch code review
  √ d87a1adecd324cd8fab3bf02c62fbff2352c1f64
    + Makes routine automagically determine target branch
    + LGTM
  √ ade437105ceece98173e13eae60fab431297f9c8
    + importppa.py also has a couple launchpad_versions_published_after()
      calls, however this is slated for removal soon. Otherwise, these
      calls would need adjusted similarly to importer.py.
    + Otherwise LGTM
  √ 371c75514e53ec4acc63c3fd48f3a31b3954e5ea
    + docstrings always welcome
    + LGTM
  √ feb96661189ea0cef2910a6faad1bf918ef57ec9
    + Refactors handling of active_series_only to pre-loop condition as
      a constructed dist_priority dictionary.
    + Adds interleave_launchpad_versions_published_after() to interleave
      the debian and ubuntu versions.
    + Adds testcase for same.
    + This routine is used to build a list of source publications
      (spi's), which are sorted by heapq.merge() using a custom key
      constructed by lookup of date created from launchpad, and dist_priority.
    + Studied dict comprehension for dist_priority creation. Looks
      correct.
    + Studied _get_cached_lp_link() behavior
    + interleave_launchpad_versions_published_after() also needs return
      value documented,
      rtype: iterator
      returns: iterator over the sorted values
    + fake_guspi() doesn't have return documented -- but it's internal to
      a test case, so I don't think it really matters. It's obvious
      enough what it's doing.
    + fake_guspi() documents 'Any' for param type, which appears to mean
      the function's parameters are not type specific. So could specify
      either a datetime object or, as in this case, integers.

review: Approve

« Back to merge proposal