Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LUCENE-10545: Allow to link to github PR from changes #854

Merged
merged 6 commits into from
May 2, 2022

Conversation

mocobeta
Copy link
Contributor

@mocobeta mocobeta commented Apr 29, 2022

changes2html already supports links to Github PRs (https://issues.apache.org/jira/browse/LUCENE-5383), but the link is obsoleted so a small modification is needed to make it work again.
With this change, developers can directly link to their pull requests from CHANGES.txt, skipping creating Jira issues. I'll also show an example in this PR.

When the change made (2014) Github was still a "third party" to Apache and the integration had been on experimental status; the situation was changed - now GitHub is the first-class code hosting service for ASF projects. I think the requirement for opening a Jira issue can be relaxed.

Here is the screenshot of generated CHANGES.html by changesToHtml task.
Screenshot from 2022-04-30 10-02-10

The "GITHUB#854" hyperlink correctly points to #854 (this pr).

@@ -53,7 +53,7 @@ Other
* LUCENE-10253: The @BadApple annotation has been removed from the test
framework. (Adrien Grand)

* LUCENE-10393: Unify binary dictionary and dictionary writer in Kuromoji and Nori.
* GITHUB-740: Unify binary dictionary and dictionary writer in Kuromoji and Nori.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mention to Github PR can be also written in a more verbose style like GITHUB Pull Request #740; I'd just prefer shorter string.

Copy link
Contributor Author

@mocobeta mocobeta Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we start to use Github issues in the future, I think this is still valid - CHANGES should directly point PRs, not issues in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. In Solr we recently fixed this to work with the new repo as well, but there we only support the GitHub #123 or gh-123. I kind of like the hash syntax, which further distinguishes it from JIRA keys (which would suggest bug number 740 in some ASF 'github" project).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janhoy thanks for your comment, I didn't think it. It makes sense to me to distinguish GitHub-style numbers from JIRA-like issue keys. I updated the regex and example.

@@ -572,7 +572,7 @@
$item =~ s{((LUCENE|SOLR|INFRA)\s+(\d{3,}))}
{<a href="${jira_url_prefix}\U$2\E-$3">$1</a>}gi;
# Link "[ github | gh ] pull request [ # ] X+" to Github pull request
$item =~ s{((?:(?:(?:github|gh)\s+)?pull\s+request\s*(?:\#?\s*)?|gh-)(\d+))}
$item =~ s{((?:(?:(?:github|gh)\s+)?pull\s+request\s*(?:\#?\s*)?|gh-|github-)(\d+))}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The acronym GH- may be okay, but GITHUB- is also good for understandability and still succinct, I think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems from your examples this is case-insensitive (ie GITHUB works) but I don't see how - the regex has no i?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's case insensitive; i appears in the next line.

@mocobeta mocobeta changed the title LUCENE-10393: allow to link github pr from changes LUCENE-10545: allow to link github pr from changes Apr 29, 2022
Copy link
Member

@rmuir rmuir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!

@mocobeta
Copy link
Contributor Author

@rmuir Thanks for confirming. I'd love to commit this in and simplify the contribution workflow.

I plan to make a post to the dev list to let others know about this change.

@mocobeta
Copy link
Contributor Author

mocobeta commented Apr 30, 2022

Thanks everyone for your feedback. I don't think this is a big deal though, I'll wait for a few more days before merging as I wrote in the mail list.

@mocobeta mocobeta changed the title LUCENE-10545: allow to link github pr from changes LUCENE-10545: Allow to link to github PR from changes May 2, 2022
@mocobeta mocobeta merged commit 5f48469 into apache:main May 2, 2022
@mocobeta mocobeta deleted the allow-github-pr-link-in-changes branch May 2, 2022 14:06
mocobeta added a commit to mocobeta/lucene that referenced this pull request May 2, 2022
wjp719 added a commit to wjp719/lucene that referenced this pull request May 10, 2022
* main: (24 commits)
  LUCENE-10532: remove @Slow annotation (apache#832)
  LUCENE-10312: Add PersianStemmer (apache#540)
  LUCENE-10558: Implement URL ctor to support classpath/module usage in Kuromoji and Nori dictionaries (main branch) (apache#871)
  LUCENE-10436: Reinstate public getdocValuesdocIdSetIterator method on DocValues (apache#869)
  Disable liftbot, we have our own tools
  LUCENE-10553: Fix WANDScorer's handling of 0 and +Infty. (apache#860)
  Make CONTRIBUTING.md a bit more succinct (apache#866)
  LUCENE-10504: KnnGraphTester to use KnnVectorQuery (apache#796)
  Add change line for LUCENE-9848
  LUCENE-9848 Sort HNSW graph neighbors for construction (apache#862)
  LUCENE-10524 Add benchmark suite details to CONTRIBUTING.md (apache#853)
  LUCENE-10552: KnnVectorQuery has incorrect equals/ hashCode (apache#859)
  LUCENE-10534: MinFloatFunction / MaxFloatFunction calls exists twice (apache#837)
  LUCENE-10188: Give SortedSetDocValues a docValueCount() (apache#663)
  Allow to link to github PR from changes (apache#854)
  LUCENE-10551: improve testing of LowercaseAsciiCompression (apache#858)
  LUCENE-10542: FieldSource exists implementations can avoid value retrieval (apache#847)
  LUCENE-10539: Return a stream of completions from FSTCompletion. (apache#844)
  gradle 7.3.3 quick upgrade (apache#856)
  LUCENE-10530: Avoid floating point precision bug in TestTaxonomyFacetAssociations (apache#848)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants