Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(36)

Issue 5704056: store: add PublishCharmsDistro function

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by niemeyer
Modified:
9 years, 9 months ago
Reviewers:
mp+95183
Visibility:
Public.

Description

https://code.launchpad.net/~niemeyer/juju/store-publish-tips/+merge/95183 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 8

Patch Set 2 : store: add PublishCharmsDistro function #

Unified diffs Side-by-side diffs Delta from patch set Stats (+414 lines, -40 lines) Patch
M store/branch_test.go View 1 5 chunks +43 lines, -40 lines 0 comments Download
A store/http_test.go View 1 1 chunk +183 lines, -0 lines 0 comments Download
A store/lpad.go View 1 1 chunk +113 lines, -0 lines 0 comments Download
A store/lpad_test.go View 1 1 chunk +68 lines, -0 lines 0 comments Download
M store/store_test.go View 1 2 chunks +7 lines, -0 lines 0 comments Download

Messages

Total messages: 17
niemeyer
Please take a look.
9 years, 9 months ago (2012-02-29 14:53:28 UTC) #1
fwereade
LGTM (also, very instructive; thanks :))
9 years, 9 months ago (2012-02-29 15:24:20 UTC) #2
rog
LGTM although i won't pretend to understand all the test logic. https://codereview.appspot.com/5704056/diff/1/store/lpad.go File store/lpad.go (right): ...
9 years, 9 months ago (2012-02-29 16:54:50 UTC) #3
niemeyer
https://codereview.appspot.com/5704056/diff/1/store/lpad.go File store/lpad.go (right): https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode50 store/lpad.go:50: return err On 2012/02/29 16:54:50, rog wrote: > i ...
9 years, 9 months ago (2012-02-29 17:27:34 UTC) #4
rog
https://codereview.appspot.com/5704056/diff/1/store/lpad.go File store/lpad.go (right): https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode50 store/lpad.go:50: return err On 2012/02/29 17:27:35, niemeyer wrote: > On ...
9 years, 9 months ago (2012-02-29 17:57:23 UTC) #5
niemeyer
https://codereview.appspot.com/5704056/diff/1/store/lpad.go File store/lpad.go (right): https://codereview.appspot.com/5704056/diff/1/store/lpad.go#newcode50 store/lpad.go:50: return err On 2012/02/29 17:57:23, rog wrote: > this ...
9 years, 9 months ago (2012-02-29 18:13:56 UTC) #6
rog
On 29 February 2012 18:13, <n13m3y3r@gmail.com> wrote: > > https://codereview.appspot.com/5704056/diff/1/store/lpad.go > File store/lpad.go (right): > ...
9 years, 9 months ago (2012-02-29 18:46:37 UTC) #7
gustavo_niemeyer.net
> if PublishCharmsDistro returns an error, it's useful for the caller > to know where ...
9 years, 9 months ago (2012-02-29 19:24:38 UTC) #8
rog
On 29 February 2012 19:24, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> if PublishCharmsDistro returns an error, ...
9 years, 9 months ago (2012-03-01 09:06:06 UTC) #9
gustavo_niemeyer.net
>> So you mean "PublishDistroCharms" is a better place to have an error >> message ...
9 years, 9 months ago (2012-03-01 13:36:20 UTC) #10
rog
On 1 March 2012 13:35, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >>> So you mean "PublishDistroCharms" is ...
9 years, 9 months ago (2012-03-01 13:58:12 UTC) #11
gustavo_niemeyer.net
> i disagree. it depends on the context. > sometimes it might be "log in ...
9 years, 9 months ago (2012-03-01 14:31:53 UTC) #12
rog
On 1 March 2012 14:31, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> i disagree. it depends on ...
9 years, 9 months ago (2012-03-01 15:22:40 UTC) #13
gustavo_niemeyer.net
> why should lpad.Login return a "launchpad login:" error > when PublishCharmsDistro should not return ...
9 years, 9 months ago (2012-03-01 15:47:51 UTC) #14
rog
On 1 March 2012 15:47, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> why should lpad.Login return a ...
9 years, 9 months ago (2012-03-01 16:12:46 UTC) #15
gustavo_niemeyer.net
On Thu, Mar 1, 2012 at 13:12, roger peppe <rogpeppe@gmail.com> wrote: >> Where did you ...
9 years, 9 months ago (2012-03-01 16:19:41 UTC) #16
niemeyer
9 years, 9 months ago (2012-03-01 20:36:45 UTC) #17
*** Submitted:

store: add PublishCharmsDistro function

R=
CC=
https://codereview.appspot.com/5704056
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b