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

Completed work for Issue #916 - Conversion to native Array.Empty<T>() #953

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

J-Exodus
Copy link
Contributor

@J-Exodus J-Exodus commented Jul 25, 2024

Conversion to native Array.Empty()

Fixes #916

Description

This is my first real PR. Always happy for feedback.

  • Removed Lucene.Net.Support.Arrays.Empty<T>() and FEATURE_ARRAYEMPTY.
  • Converted all Arrays.Empty<T>() to Array.Empty<T>() and added using System; where required.
  • Removed using Lucene.Net.Support; statements that were no longer required.
  • In FieldComparitor.cs converted MISSING_BTYES and NON_MISSING_BYTES to Array.Empty<bytes>()

Conducted unit tests - all passed. Unsure correct format to attach results.

Unfortunately, my linter has picked up the extra white spaces in the files I updated, which are marked as individual changes. If this effects the ability to review and merge this PR, please let me know and I'll redo it all with the linter off.

Removed ```Lucene.Net.Support.Arrays.Empty<T>()``` and ```FEATURE_ARRAYEMPTY```.
Converted all ```Arrays.Empty<T>()``` to ```Array.Empty<T>()``` and added ```using System;``` where required.
Removed ```using Lucene.Net.Support;``` statements that were no longer required.
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 and your willingness to help make our project better.

There are a few changes we would like you to make before we can accept this PR. First, please see the comments inline.

In addition to those changes, this task is to remove FEATURE_ARRAYEMPTY from the codebase. But it still exists in these 2 places. Please remove these lines.

https://github.com/J-Exodus/lucenenet/blob/issue-916/Directory.Build.targets#L55
https://github.com/J-Exodus/lucenenet/blob/issue-916/Directory.Build.targets#L116-L122

Rectified Pull request issues as identified by NightOwl888:
 - Removed to missed references to `FEATURE_ARRAYEMPTY'
 - Reverted `FieldComparator.cs` back to original state
 - Corrected sort order for using statements
 - Reverted indentation on `JavascriptLexer.cs`
@J-Exodus
Copy link
Contributor Author

Thanks for your patience in providing feedback for this PR. I have corrected the issues as identified. This one should now be good to go.

@paulirwin paulirwin dismissed NightOwl888’s stale review August 13, 2024 16:45

Changes have been addressed by PR author

@paulirwin paulirwin merged commit 09a6de9 into apache:master Aug 13, 2024
199 checks passed
@J-Exodus J-Exodus deleted the issue-916 branch August 13, 2024 18:58
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.

Replace Lucene.Net.Support.Arrays.Empty<T> with System.Array.Empty<T>
3 participants