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

Test review A-D, #259 #1018

Merged
merged 15 commits into from
Nov 18, 2024
Merged

Test review A-D, #259 #1018

merged 15 commits into from
Nov 18, 2024

Conversation

paulirwin
Copy link
Contributor

@paulirwin paulirwin commented Nov 11, 2024

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Test review for core A-D tests

Fixes #259 (as the final PR for this issue)

Description

Add missing comments and attributes, match formatting, remove unnecessary code, etc.

@paulirwin paulirwin mentioned this pull request Nov 11, 2024
11 tasks
@paulirwin paulirwin marked this pull request as ready for review November 13, 2024 00:07
Copy link
Contributor

@NightOwl888 NightOwl888 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 the PR. The final PR of #259. Nice work!

There are a few changes to make to this before we can merge it. Plus, it came to my attention that there are some other things we need to review and fix before the release that we will need some issues opened for. Please see my comments inline.

src/Lucene.Net.Tests/Analysis/TestNumericTokenStream.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Analysis/TestNumericTokenStream.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Analysis/TestNumericTokenStream.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Codecs/Lucene41/TestForUtil.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Document/TestBinaryDocument.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Document/TestBinaryDocument.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

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

Looks good.

Although, we may need to retitle this ReusableStringReader changed from public to internal because there is only 1 breaking change and it is more sensible to put that in the release notes than Test review A-D, #259. Or, we can move the breaking change to a different PR, which would probably look better in the release notes. Either way is fine.

@NightOwl888 NightOwl888 added the notes:breaking-change Has changes that will break backward compatibility label Nov 17, 2024
@paulirwin paulirwin removed the notes:breaking-change Has changes that will break backward compatibility label Nov 18, 2024
@paulirwin
Copy link
Contributor Author

Reverted breaking change and removed label, will put in a separate PR with that label. Thanks!

@paulirwin paulirwin added the notes:improvement An enhancement to an existing feature label Nov 18, 2024
@paulirwin paulirwin merged commit c4ab72c into apache:master Nov 18, 2024
199 checks passed
@paulirwin paulirwin deleted the test-review-a-d branch November 18, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:improvement An enhancement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review tests for Lucene.Net assembly
2 participants