)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9453,"name":"James Polley","email":"jp@jamezpolley.com","username":"tchaypo"},"change_message_id":"265c95b4a641ea9264add16c777f256ba21ee82f","unresolved":false,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: Ib7db0c8ab78e3cb700671f6a123ec603b4dbfdbe"},{"line_number":15,"context_line":"Closes-bug: 1493735"},{"line_number":16,"context_line":"Co-Authored-By: Sachi King \u003cnakato@nakato.io\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"7a2fa921_7d145243","line":16,"updated":"2015-10-07 10:41:50.000000000","message":"IF there\u0027s another revision, I\u0027d suggest swapping Author and Co-Authored-By - when the co-author wrote 7700% more lines than the author, things are a bit skewiff.","commit_id":"36a2bf130999f06562b5eb063e1c49ae2d6e35a7"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"e69f188bd5277eaae498378294a06140ccb5b198","unresolved":false,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: Ib7db0c8ab78e3cb700671f6a123ec603b4dbfdbe"},{"line_number":15,"context_line":"Closes-bug: 1493735"},{"line_number":16,"context_line":"Co-Authored-By: Sachi King \u003cnakato@nakato.io\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"7a2fa921_6aeb0db1","line":16,"in_reply_to":"7a2fa921_7d145243","updated":"2015-10-07 18:43:55.000000000","message":"Meh - thats really not important: the authors are a set from a tooling perspective.","commit_id":"36a2bf130999f06562b5eb063e1c49ae2d6e35a7"}],"pbr/tests/test_integration.py":[{"author":{"_account_id":9453,"name":"James Polley","email":"jp@jamezpolley.com","username":"tchaypo"},"change_message_id":"265c95b4a641ea9264add16c777f256ba21ee82f","unresolved":false,"context_lines":[{"line_number":156,"context_line":""},{"line_number":157,"context_line":"class TestInstallWithoutPbr(base.BaseTestCase):"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    @testtools.skipUnless("},{"line_number":160,"context_line":"        os.environ.get(\u0027PBR_INTEGRATION\u0027, None) \u003d\u003d \u00271\u0027,"},{"line_number":161,"context_line":"        \u0027integration tests not enabled\u0027)"},{"line_number":162,"context_line":"    def test_install_without_pbr(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_42abb11d","line":159,"updated":"2015-10-07 10:41:50.000000000","message":"I don\u0027t think the skipUnless is needed here; this test shouldn\u0027t take as long to run as the tests above.\n\nOn the other hand, it is an integration test, and that flag does control when integration tests run...\n\nSo I can live it this either way, but my gut tells me it would be better to surprise people by running this test, than to surprise them by not testing this behaviour.","commit_id":"36a2bf130999f06562b5eb063e1c49ae2d6e35a7"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"e69f188bd5277eaae498378294a06140ccb5b198","unresolved":false,"context_lines":[{"line_number":156,"context_line":""},{"line_number":157,"context_line":"class TestInstallWithoutPbr(base.BaseTestCase):"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    @testtools.skipUnless("},{"line_number":160,"context_line":"        os.environ.get(\u0027PBR_INTEGRATION\u0027, None) \u003d\u003d \u00271\u0027,"},{"line_number":161,"context_line":"        \u0027integration tests not enabled\u0027)"},{"line_number":162,"context_line":"    def test_install_without_pbr(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_4a0df18e","line":159,"in_reply_to":"7a2fa921_42abb11d","updated":"2015-10-07 18:43:55.000000000","message":"It\u0027s absolutely needed. This cannot run without PyPI available, e.g. on a train. We should document this and stop it being tribal knowledge - if you wanted to put some effort into developer guide docs that would rock.","commit_id":"36a2bf130999f06562b5eb063e1c49ae2d6e35a7"},{"author":{"_account_id":9453,"name":"James Polley","email":"jp@jamezpolley.com","username":"tchaypo"},"change_message_id":"265c95b4a641ea9264add16c777f256ba21ee82f","unresolved":false,"context_lines":[{"line_number":159,"context_line":"    @testtools.skipUnless("},{"line_number":160,"context_line":"        os.environ.get(\u0027PBR_INTEGRATION\u0027, None) \u003d\u003d \u00271\u0027,"},{"line_number":161,"context_line":"        \u0027integration tests not enabled\u0027)"},{"line_number":162,"context_line":"    def test_install_without_pbr(self):"},{"line_number":163,"context_line":"        # Test easy-install of a thing that depends on a thing using pbr"},{"line_number":164,"context_line":"        tempdir \u003d self.useFixture(fixtures.TempDir()).path"},{"line_number":165,"context_line":"        # A directory containing sdists of the things we\u0027re going to depend on"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_622b4d95","line":162,"updated":"2015-10-07 10:41:50.000000000","message":"Pedantry (not worth a -1) - something like \"test_pbr_gets_installed_by_surprise\" might be a better name here, since by the end of the test pbr is actually installed","commit_id":"36a2bf130999f06562b5eb063e1c49ae2d6e35a7"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"e69f188bd5277eaae498378294a06140ccb5b198","unresolved":false,"context_lines":[{"line_number":159,"context_line":"    @testtools.skipUnless("},{"line_number":160,"context_line":"        os.environ.get(\u0027PBR_INTEGRATION\u0027, None) \u003d\u003d \u00271\u0027,"},{"line_number":161,"context_line":"        \u0027integration tests not enabled\u0027)"},{"line_number":162,"context_line":"    def test_install_without_pbr(self):"},{"line_number":163,"context_line":"        # Test easy-install of a thing that depends on a thing using pbr"},{"line_number":164,"context_line":"        tempdir \u003d self.useFixture(fixtures.TempDir()).path"},{"line_number":165,"context_line":"        # A directory containing sdists of the things we\u0027re going to depend on"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_6aa30d50","line":162,"in_reply_to":"7a2fa921_622b4d95","updated":"2015-10-07 18:43:55.000000000","message":"mmm, I think we could wiggle on the name for ages. The comment here is the actual key - its easy-install (which gets into transitively used setuptools instances) + pbr being not the first-thing-triggered that exposes the bug. \n\nAlso, as a general rule \u0027surprise\u0027 being a subjective thing is not a good naming strategy - its implications are not derivable for other people without a -lot- of mental state.","commit_id":"36a2bf130999f06562b5eb063e1c49ae2d6e35a7"},{"author":{"_account_id":9453,"name":"James Polley","email":"jp@jamezpolley.com","username":"tchaypo"},"change_message_id":"265c95b4a641ea9264add16c777f256ba21ee82f","unresolved":false,"context_lines":[{"line_number":210,"context_line":"            f.write(textwrap.dedent(\"\"\"\\"},{"line_number":211,"context_line":"                pbr"},{"line_number":212,"context_line":"                \"\"\"))"},{"line_number":213,"context_line":"        with open(os.path.join(req_pkg_dir, \u0027pkgReq/__init__.py\u0027), \u0027wt\u0027) as f:"},{"line_number":214,"context_line":"            f.write(textwrap.dedent(\"\"\"\\"},{"line_number":215,"context_line":"                print(\"FakeTest loaded and ran\")"},{"line_number":216,"context_line":"                \"\"\"))"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_42597159","line":213,"range":{"start_line":213,"start_character":45,"end_line":213,"end_character":51},"updated":"2015-10-07 10:41:50.000000000","message":"There\u0027s a / in the middle of the filename here.\n\nIf we\u0027re comfortable assuming / is the path seperator, invoking os.path.join is a waste of time. If os.path.join is required, this should be (req_pkg_dir, \"pkgReq\", \"__init__.py\")","commit_id":"36a2bf130999f06562b5eb063e1c49ae2d6e35a7"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"e69f188bd5277eaae498378294a06140ccb5b198","unresolved":false,"context_lines":[{"line_number":210,"context_line":"            f.write(textwrap.dedent(\"\"\"\\"},{"line_number":211,"context_line":"                pbr"},{"line_number":212,"context_line":"                \"\"\"))"},{"line_number":213,"context_line":"        with open(os.path.join(req_pkg_dir, \u0027pkgReq/__init__.py\u0027), \u0027wt\u0027) as f:"},{"line_number":214,"context_line":"            f.write(textwrap.dedent(\"\"\"\\"},{"line_number":215,"context_line":"                print(\"FakeTest loaded and ran\")"},{"line_number":216,"context_line":"                \"\"\"))"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a2fa921_8a646922","line":213,"in_reply_to":"7a2fa921_42597159","updated":"2015-10-07 18:43:55.000000000","message":"I\u0027m pretty sure its fine. os.path.join\u0027s key thing is that if req_pkg_dir ends with \u0027/\u0027 it will do the right thing and not end up with // in the path. I see no harm here - OTOH I might be horribly wrong, but its working reliably in our test environments, and I\u0027m sure we will have issues on Windows if/when we get a test variant there - but I don\u0027t see any point in borrowing such trouble. So I\u0027d rather our cultural norm on review be relaxed about this.\n\n(And FWIW os.path.join is deliberately intended to support joining two arbitrary paths, not just single-directory-components - see its docstring and source).","commit_id":"36a2bf130999f06562b5eb063e1c49ae2d6e35a7"}]}