* bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud @ 2016-07-11 15:05 Teodor Zlatanov 2016-07-13 14:20 ` Ted Zlatanov 0 siblings, 1 reply; 8+ messages in thread From: Teodor Zlatanov @ 2016-07-11 15:05 UTC (permalink / raw) To: 23945 I've published the proposed changes to the Gnus Cloud code in the branch `scratch/gnus-cloud' and would like to request a code review. It's a single commit which I've tested over the last 2 weeks with the help of other Gnus users. I'll add documentation if the code is acceptable. Thank you Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud 2016-07-11 15:05 bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud Teodor Zlatanov @ 2016-07-13 14:20 ` Ted Zlatanov 2016-07-18 14:08 ` Ted Zlatanov 0 siblings, 1 reply; 8+ messages in thread From: Ted Zlatanov @ 2016-07-13 14:20 UTC (permalink / raw) To: 23945, Lars Magne Ingebrigtsen On Mon, 11 Jul 2016 11:05:55 -0400 Teodor Zlatanov <tzz@lifelogs.com> wrote: TZ> I've published the proposed changes to the Gnus Cloud code in the branch TZ> `scratch/gnus-cloud' and would like to request a code review. It's a TZ> single commit which I've tested over the last 2 weeks with the help of TZ> other Gnus users. TZ> I'll add documentation if the code is acceptable. Lars? You look like a good reviewer. There is one piece I remembered was missing: making the cloud host persistent. After restarting Gnus, you have to set the gnus-cloud-method with `gnus-server-toggle-cloud-method-server' again. How would you suggest to save that, with a defcustom in gnus-cloud or in the individual server's parameters? Right now it's just (defvar gnus-cloud-method nil "The IMAP select method used to store the cloud data.") If I save it in the individual server's parameters there's a possibility that two servers will have the cloud host flag accidentally. Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud 2016-07-13 14:20 ` Ted Zlatanov @ 2016-07-18 14:08 ` Ted Zlatanov 2016-07-18 23:18 ` npostavs 0 siblings, 1 reply; 8+ messages in thread From: Ted Zlatanov @ 2016-07-18 14:08 UTC (permalink / raw) To: 23945; +Cc: Lars Magne Ingebrigtsen On Wed, 13 Jul 2016 10:20:12 -0400 Ted Zlatanov <tzz@lifelogs.com> wrote: TZ> On Mon, 11 Jul 2016 11:05:55 -0400 Teodor Zlatanov <tzz@lifelogs.com> wrote: TZ> I've published the proposed changes to the Gnus Cloud code in the branch TZ> `scratch/gnus-cloud' and would like to request a code review. It's a TZ> single commit which I've tested over the last 2 weeks with the help of TZ> other Gnus users. TZ> I'll add documentation if the code is acceptable. TZ> There is one piece I remembered was missing: making the cloud host TZ> persistent. I converted `gnus-cloud-method' to a defcustom and added the necessary code to set it, resolving this question. Since no one has been interested in reviewing this code, I will merge it tomorrow. Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud 2016-07-18 14:08 ` Ted Zlatanov @ 2016-07-18 23:18 ` npostavs 2016-07-19 14:50 ` Ted Zlatanov 0 siblings, 1 reply; 8+ messages in thread From: npostavs @ 2016-07-18 23:18 UTC (permalink / raw) To: 23945 Ted Zlatanov <tzz@lifelogs.com> writes: > On Wed, 13 Jul 2016 10:20:12 -0400 Ted Zlatanov <tzz@lifelogs.com> wrote: > > TZ> On Mon, 11 Jul 2016 11:05:55 -0400 Teodor Zlatanov <tzz@lifelogs.com> wrote: > TZ> I've published the proposed changes to the Gnus Cloud code in the branch > TZ> `scratch/gnus-cloud' and would like to request a code review. It's a > TZ> single commit which I've tested over the last 2 weeks with the help of > TZ> other Gnus users. > > TZ> I'll add documentation if the code is acceptable. > > TZ> There is one piece I remembered was missing: making the cloud host > TZ> persistent. > > I converted `gnus-cloud-method' to a defcustom and added the necessary > code to set it, resolving this question. > > Since no one has been interested in reviewing this code, I will merge it > tomorrow. Perhaps this is partly because I'm not familiar with the code (or what "Gnus Cloud" is), but it seems to me that you're missing a good summary line explaining what any of these changes are for. The bug title and commit message summary line mentions only "Gnus Cloud work". What "work"? This feels actively reviewer-hostile. The 2nd commit titled "Minor gnus-cloud UI improvements" is a bit better (at least we can tell it's about UI), though adding 20 or so "Merge branch 'master' of git.sv.gnu.org:/srv/git/emacs" in between doesn't help much either. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud 2016-07-18 23:18 ` npostavs @ 2016-07-19 14:50 ` Ted Zlatanov 2016-07-19 20:15 ` Ted Zlatanov 0 siblings, 1 reply; 8+ messages in thread From: Ted Zlatanov @ 2016-07-19 14:50 UTC (permalink / raw) To: npostavs; +Cc: 23945 On Mon, 18 Jul 2016 19:18:09 -0400 npostavs@users.sourceforge.net wrote: >> I converted `gnus-cloud-method' to a defcustom and added the necessary >> code to set it, resolving this question. >> >> Since no one has been interested in reviewing this code, I will merge it >> tomorrow. n> Perhaps this is partly because I'm not familiar with the code (or what n> "Gnus Cloud" is), but it seems to me that you're missing a good summary n> line explaining what any of these changes are for. The bug title and n> commit message summary line mentions only "Gnus Cloud work". What n> "work"? This feels actively reviewer-hostile. You're right. The history of this work goes back at least 4 years in the Gnus mailing list so I was hoping Lars or someone else familiar with that history would jump in. You can start with the most recent log of work I did: http://thread.gmane.org/gmane.emacs.gnus.general/85543/focus=87091 I will write docs for the feature, but simply wasn't sure about some key aspects of it that depended on the code. Please note I did add docstrings for most of the functions, especially the interactive ones. But I agree that's no excuse for a lack of good commit messages. n> The 2nd commit titled "Minor gnus-cloud UI improvements" is a bit better n> (at least we can tell it's about UI), though adding 20 or so "Merge n> branch 'master' of git.sv.gnu.org:/srv/git/emacs" in between doesn't n> help much either. Well, that's an issue with the source code repo. I've mentioned it before. Here's my rant: I merge the master branch often because I have been using the gnus-cloud branch and want to make sure it's not broken against master. I also test and use many other new features. I really don't want to start working outside the master branch. But I can't rebase and push again, because the repo doesn't support non-fast-forward pushes. I have to delete and recreate the branch (notification emails x2, and a little bit of manual labor). But also every time I do a push with merges, notification e-mails come out for my changes plus everything merged. I can't win either way. So the bottom line for me is, I'd really like notification e-mails not to go out for "scratch/*" branches, or to be able to do a non-fast-forward push to my scratch branch. Since neither is possible, I'm doing the best I can for now. I am strongly considering using Github next time (Git supports multiple remotes for a branch, so this is not too hard) and saving myself all this trouble. You can use `git log --no-merges' meanwhile to filter out those merge commits. Sorry for the inconvenience. Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud 2016-07-19 14:50 ` Ted Zlatanov @ 2016-07-19 20:15 ` Ted Zlatanov 2016-07-20 1:36 ` Noam Postavsky 0 siblings, 1 reply; 8+ messages in thread From: Ted Zlatanov @ 2016-07-19 20:15 UTC (permalink / raw) To: npostavs; +Cc: 23945 On Tue, 19 Jul 2016 10:50:07 -0400 Ted Zlatanov <tzz@lifelogs.com> wrote: TZ> You're right. The history of this work goes back at least 4 years in the TZ> Gnus mailing list so I was hoping Lars or someone else familiar with TZ> that history would jump in. You can start with the most recent log of TZ> work I did: TZ> http://thread.gmane.org/gmane.emacs.gnus.general/85543/focus=87091 TZ> I will write docs for the feature, but simply wasn't sure about some key TZ> aspects of it that depended on the code. I rebased and added docs for this feature. So now there's just three commits in the scratch/gnus-cloud branch. Let me know, based on the docs, what the commit message should say. I can't think of a good summary, since the work is really widespread in order to make this package useful. Thanks Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud 2016-07-19 20:15 ` Ted Zlatanov @ 2016-07-20 1:36 ` Noam Postavsky 2016-07-20 12:55 ` Ted Zlatanov 0 siblings, 1 reply; 8+ messages in thread From: Noam Postavsky @ 2016-07-20 1:36 UTC (permalink / raw) To: Ted Zlatanov; +Cc: 23945 On Tue, Jul 19, 2016 at 4:15 PM, Ted Zlatanov <tzz@lifelogs.com> wrote: > On Tue, 19 Jul 2016 10:50:07 -0400 Ted Zlatanov <tzz@lifelogs.com> wrote: > > TZ> You're right. The history of this work goes back at least 4 years in the > TZ> Gnus mailing list so I was hoping Lars or someone else familiar with > TZ> that history would jump in. You can start with the most recent log of > TZ> work I did: > > TZ> http://thread.gmane.org/gmane.emacs.gnus.general/85543/focus=87091 > > TZ> I will write docs for the feature, but simply wasn't sure about some key > TZ> aspects of it that depended on the code. > > I rebased and added docs for this feature. So now there's just three > commits in the scratch/gnus-cloud branch. > > Let me know, based on the docs, what the commit message should say. I > can't think of a good summary, since the work is really widespread in > order to make this package useful. Maybe something like "Bring gnus-cloud.el into working order", I gather from that thread that it was broken before, right? That's still not very descriptive but it looks like there's just a lot of little changes that aren't really tied together. Perhaps it would be better broken up into more commits (though that's probably not worth the trouble at this point). ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud 2016-07-20 1:36 ` Noam Postavsky @ 2016-07-20 12:55 ` Ted Zlatanov 0 siblings, 0 replies; 8+ messages in thread From: Ted Zlatanov @ 2016-07-20 12:55 UTC (permalink / raw) To: Noam Postavsky; +Cc: 23945-done On Tue, 19 Jul 2016 21:36:12 -0400 Noam Postavsky <npostavs@users.sourceforge.net> wrote: NP> On Tue, Jul 19, 2016 at 4:15 PM, Ted Zlatanov <tzz@lifelogs.com> wrote: >> Let me know, based on the docs, what the commit message should say. I >> can't think of a good summary, since the work is really widespread in >> order to make this package useful. NP> Maybe something like "Bring gnus-cloud.el into working order", I NP> gather from that thread that it was broken before, right? That's still NP> not very descriptive but it looks like there's just a lot of little NP> changes that aren't really tied together. Perhaps it would be better NP> broken up into more commits (though that's probably not worth the NP> trouble at this point). All right... I squashed with the message you suggested. It's pushed so I'll call this review done. Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-07-20 12:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-11 15:05 bug#23945: 25.1.50; Request for review: Gnus Cloud work in scratch/gnus-cloud Teodor Zlatanov 2016-07-13 14:20 ` Ted Zlatanov 2016-07-18 14:08 ` Ted Zlatanov 2016-07-18 23:18 ` npostavs 2016-07-19 14:50 ` Ted Zlatanov 2016-07-19 20:15 ` Ted Zlatanov 2016-07-20 1:36 ` Noam Postavsky 2016-07-20 12:55 ` Ted Zlatanov
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).