lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Risden <compuwizard...@gmail.com>
Subject Re: Baby steps as new committer
Date Tue, 15 Aug 2017 22:44:09 GMT
> > On Tue, Aug 15, 2017 at 2:57 PM Toke Eskildsen <toes@kb.dk> wrote:
> > > Erick's explanation is perfect I think. I would only add that an issue/patch
> > > combination will typically bring more eyes to the code. If you seek
> > > feedback and want to ensure the changes are not breaking things it is
> > > the way to go.
>> Thank you, and thank you Erick. I am a bit surprised that uploading a patch is the
preferred way, at least by you two. But I guess most committers have scripts in place to ease
download/diff/apply?
> I don't have such scripts :-)  If I see a .patch file I want to review, I usually review
it by itself (don't download or apply).  Of course this kinda sucks for a bunch of obvious
reasons.

I typically do the following to review patches:

* Download patch from JIRA (defaults to ~/Downloads)
* git checkout branch_patch_is_based_on
* git checkout -b JIRA (ie: SOLR-10000)
* git apply -p1 ~/Downloads/$(git rev-parse --abbrev-ref HEAD).patch
** This takes the current branch name (ie: SOLR-10000) and will apply
the right patch

This is as far as I've automated it but it works pretty well for my
use cases. Sometimes I will push this to my github fork of the Apache
Lucene/Solr repo and do a compare.

Pull requests are nice if you don't have multiple people trying to
contribute at the same time. It is hard for multiple people to use PRs
to iterate on the same JIRA. Github has ways to allow contributors and
others to push to other people's PRs but not sure it is enabled for
the Apache Lucene/Solr repo. For diffing/reviewing PRs are nice and
Github makes it easy to leave review comments.

Kevin Risden


On Tue, Aug 15, 2017 at 5:07 PM, David Smiley <david.w.smiley@gmail.com> wrote:
> On Tue, Aug 15, 2017 at 2:57 PM Toke Eskildsen <toes@kb.dk> wrote:
>>
>> Dawid Weiss <dawid.weiss@gmail.com> wrote:
>> > Erick's explanation is perfect I think. I would only add that an
>> > issue/patch
>> > combination will typically bring more eyes to the code. If you seek
>> > feedback and want to ensure the changes are not breaking things it is
>> > the way to go.
>>
>> Thank you, and thank you Erick. I am a bit surprised that uploading a
>> patch is the preferred way, at least by you two. But I guess most committers
>> have scripts in place to ease download/diff/apply?
>
>
> I don't have such scripts :-)  If I see a .patch file I want to review, I
> usually review it by itself (don't download or apply).  Of course this kinda
> sucks for a bunch of obvious reasons.
>
>>
>> I really like the GitHub pull-request-mechanism, but as I am the one
>> asking for review of my code (in this case at least), I will of course use
>> the method with the highest chance of getting a review.
>
>
> I certainly favor GitHub pull requests way more than a patch, in terms of
> the reviewer experience.
>
> I suppose I tend to not do this myself for my own issues only out of habit.
> If there ultimately isn't any review, no path is particularly easier than
> the other, I think.  I should try to do GitHub PRs more... though I wonder
> if it will result in more annoying dev list traffic?
>
>> Related to that I am unsure about Affect/Fix versions in JIRA. The
>> SOLR-11240 issue is present in Solr 5+, so I just picked the latest released
>> minor version for 5 & 6 + master. Was that correct?
>
>
> We just had some email thread(s) about these fields on the dev list.
>
> The "affects version" doesn't get much scrutiny... how you filled it seems
> fine.  Now the "fix version/s" matters more.  I believe 5.x is very EOL so
> simply don't put any 5.x version in "fix version/s" unless you really truly
> want to commit it there despite the EOL status.  6.6 was wrong because 6.6
> shipped already.  I think 6.7 is appropriate since this is a minor
> improvement you're talking about here.  Again, putting any 6.x version here
> implies you are going to backport it.  And you missed 7.x since most new
> features will get committed to this version series, in addition to master
> which you already have listed.  Essentially for every branch you are going
> to commit this to, there should be a corresponding fix-version.  The actual
> branch name and JIRA "version" values aren't identically aligned but they
> are similar.
>
> ~ David
> --
> Lucene/Solr Search Committer, Consultant, Developer, Author, Speaker
> LinkedIn: http://linkedin.com/in/davidwsmiley | Book:
> http://www.solrenterprisesearchserver.com

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message