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

Deprecate Operations.concat(a1, a2) and Operations.union(a1, a2) #14209

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rmuir
Copy link
Member

@rmuir rmuir commented Feb 6, 2025

These algorithms run in linear time: it is trappy to offer two-arg options: they encourage users to use them in a loop and create quadratic time.

Send a strong signal to the user's editor/IDE to avoid trouble in the first place:

  • Deprecate two-arg methods in favor of the List-based versions
  • Rename parameter 'l' to 'list'
  • Add javadoc @param to parameter 'list'

Closes #14202

These algorithms run in linear time: it is trappy to offer two-arg
options: they encourage users to use them in a loop and create quadratic
time.

Send a strong signal to the user's editor/IDE to avoid trouble in the
first place:
* Deprecate two-arg methods in favor of the List-based versions
* Rename parameter 'l' to 'list'
* Add javadoc @param to parameter 'list'
Copy link
Member

@mikemccand mikemccand left a comment

Choose a reason for hiding this comment

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

Yay. Thanks @rmuir.

@rmuir
Copy link
Member Author

rmuir commented Feb 6, 2025

I will look into the test fail, I did run them many times, but I was also pretty aggressive about trying to cleanup tests, so that we can just remove the deprecations in a followup commit, and improve efficiency in some places too.

Code in the tests and suggester were impacted in this way.

@rmuir
Copy link
Member Author

rmuir commented Feb 6, 2025

The problem is not related to this PR from what I can tell, the issue is that concatenate() turns an NFA into a DFA and this fails the test.

I can reproduce it in main: I will deal with it separately.

@rmuir
Copy link
Member Author

rmuir commented Feb 6, 2025

#14210

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.

This looks good to me, though I wonder if we really need the deprecation dance as there are internal APIs (expert at best), so we could proceed with the removal directly?

@rmuir
Copy link
Member Author

rmuir commented Feb 6, 2025

To me the deprecation is easy enough, developer is usually responsive to such things.

It comes across different than just a hard break in a few ways, usually you question why the previous thing was deprecated and it causes you to take a second look. Versus just trying to make the code compile again. I'd rather us give the helpful nudge to make the code more efficient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate Operations.concatenate(a1, a2) and Operations.union(a1, a2)
3 participants