NightOwl888 commented on PR #948: URL: https://github.com/apache/lucenenet/pull/948#issuecomment-2428428333
@paulirwin - Thanks for finding that PR. The fact that a link or mention of it was missing from both the issue and PR was the primary thing holding this up. I see a few issues that should be addressed before merging it. - [ ] 1. The production change is missing a `LUCENENET` comment indicating where the change (which diverges from 4.8.0) came from. Ideally, there would be a link back to the PR or commit where the code was ported from. - [ ] 2. The name of the test method should be Pascal case. NUnit doesn't automatically detect test methods based on a naming convention, so there is no need to stick with that casing in .NET. - [ ] 3. The test method and `GetCommitData()` method are not in the same order in the file as they were in Lucene. In Lucene, `TestVariableSchema()` (the following test in this PR) is on [line 593](https://github.com/Shibi-bala/lucene/blob/e98cc333e2131ca9d39f81a2da9fed5ab0d1d5bb/lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java#L593) and `TestGetCommitDataFromOldSnapshot()` is on [line 2064](https://github.com/Shibi-bala/lucene/blob/e98cc333e2131ca9d39f81a2da9fed5ab0d1d5bb/lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java#L2064). - [ ] 4. The original test used `getLiveCommitData()`, not `getCommitData()`. It's fine if `setLiveCommitData()` doesn't exist in 4.8.0, but we should `LUCENENET` comment to make it clear why we diverged (and that it has diverged) on each of the lines that call `GetCommitData()` and `SetCommitData()`, since these will need to be changed back when we upgrade. - [ ] 5. The original test used [`newSnapshotIndexWriterConfig(null)`](https://github.com/Shibi-bala/lucene/blob/e98cc333e2131ca9d39f81a2da9fed5ab0d1d5bb/lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java#L935-L940). We should port that method in `LuceneTestCase` rather than inlining the configuration, and add a `LUCENENET` comment indicating where the diverging method came from. - [ ] 6. The `TestGetCommitDataFromOldSnapshot()` and `GetCommitData()` methods also don't have a `LUCENENET` comment indicating where these diverging changes came from. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
