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

Improve MIGRATE.md around analyzers artifacts. #488

Merged
merged 3 commits into from
Nov 29, 2021

Conversation

rmuir
Copy link
Member

@rmuir rmuir commented Nov 29, 2021

Move this to the very top of MIGRATE, the user needs to first be able to pull in the artifacts, before doing anything else like trying to compile, deal with renamed classes, etc.

Add a table of each package that got moved, with explicit old and new names. Hopefully it helps search engines and users.

@jpountz I'd like to backport this to 9.0 if possible, since we are respinning for module names anyway. It is low risk.

Description

Please provide a short description of the changes you're making with this pull request.

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.

Move this to the very top of MIGRATE, the user needs to first be able to
pull in the artifacts, before doing anything else like trying to
compile, deal with renamed classes, etc.

Add a table of each package that got moved, with explicit old and new
names. Hopefully it helps search engines and users.
@rmuir rmuir requested review from dweiss and uschindler November 29, 2021 21:27
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Feel free to go ahead with backporting.

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

Have you compared all Maven artifacts, also the ones not related to analysis?

Otherwise looks fine.

@uschindler
Copy link
Contributor

A separate note: The new name is also more conforming what the modules relay do: They are not only "analyzers", those are compoents for "analysis" of text while indexing/searching lucene. So new name is better. Maybe add this to explanation.

@uschindler
Copy link
Contributor

Table looks much better now. Thanks!

@rmuir
Copy link
Member Author

rmuir commented Nov 29, 2021

Sorry for all the commits, I wanted to try to make this easy to read and prominent, and linked from the README too for more visibility.

I realize the MIGRATE.md is an unstructured list, but there's an advantage to listing some stuff at the top (esp. if it is likely to impact most users that upgrade). I don't want to hold up the 9.0 release, but maybe for the next one we can improve it to be better, some ideas:

  • avoid usage of abbreviations in our MIGRATE notes such as o.a.l.a.util.TokenizerFactory
  • avoid lists of from-to stuff and use tables (like this one as an example)
  • structure the high-impacting stuff such as package and class renames at the top.

@uschindler
Copy link
Contributor

Originally I made the MIGRATE.md a markdown file to have all formatting possibilities. The unordered list was just a "quick conversion" from the old format introduced in Lucene 4.0.

The Markdown converter accepts all markdown in the gradlew documentation output and also expands LUCENE/SOLR issue numbers (and makes them clickable): https://github.com/apache/lucene/blob/main/gradle/documentation/markdown.gradle

@rmuir
Copy link
Member Author

rmuir commented Nov 29, 2021

A separate note: The new name is also more conforming what the modules relay do: They are not only "analyzers", those are compoents for "analysis" of text while indexing/searching lucene. So new name is better. Maybe add this to explanation.

I'd rather not mix in justification/reasoning for any changes in this file, I think it adds noise. Most users will be annoyed with us regardless :) I think this file should just be simple hints of how to fix your code? We list the JIRA issue for each change already, in case an interested party wants to drill down to background discussion of why changes were done, or get any more detailed information.

@uschindler
Copy link
Contributor

This was just meant as a replacement for this text: "and are now consistent with repository module 'analysis'". This does not sound like a acceptable explanation to an annoyed user, so my idea was to just say: "better name because it does more than providing analyzers".

But all fine, was just my 2ct.

@rmuir rmuir merged commit 5aa9da9 into apache:main Nov 29, 2021
@uschindler
Copy link
Contributor

Hi Robert,
I built the documentation with "gradlew documentation" and noticed that tables were not enabled in the markdown converter. It now looks not very well.

If you don't mind I will add a change to fix the converter:
image

So please wait a bit with merging!

asfgit pushed a commit that referenced this pull request Nov 29, 2021
* Improve MIGRATE.md around analyzers artifacts.

Move this to the very top of MIGRATE, the user needs to first be able to
pull in the artifacts, before doing anything else like trying to
compile, deal with renamed classes, etc.

Add a table of each package that got moved, with explicit old and new
names. Hopefully it helps search engines and users.

Link to MIGRATE.md explicitly from README.md
asfgit pushed a commit that referenced this pull request Nov 29, 2021
* Improve MIGRATE.md around analyzers artifacts.

Move this to the very top of MIGRATE, the user needs to first be able to
pull in the artifacts, before doing anything else like trying to
compile, deal with renamed classes, etc.

Add a table of each package that got moved, with explicit old and new
names. Hopefully it helps search engines and users.

Link to MIGRATE.md explicitly from README.md
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.

3 participants