Github user synhershko commented on the issue:
https://github.com/apache/lucenenet/pull/176
Hi, sorry it took me so long to review. I wanted to review properly and
give proper feedback to complement your hard (and very professional!) work here.
So,
1. This makes sense. This is in-line with our .NETification spirit, and as
far as we don't remove important functionality I'd say this makes sense and can
be removed altogether - or changed to work in a way that makes sense.
2. This makes sense as well. We took this approach in several other places
in the code base. I agree the guideline here should be code readability /
usability / fluidity.
3. Using an Enum does make sense, and indeed better to wait for tests to
stabilize.
4. This might diverge from other places in our codebase. Why is it public
API anyway? I can't really see anyone using it, except from people trying to
customize the SimpleQueryParser - which is simplistic in it's nature. I'd say
let's make it nullable and either accept it's a public API with not much
expected usage, or hide it as non-public API. WDYT?
5. From my familiarity with the Java code base, you pass a flag to enable a
feature. I think it's just confusing test names, and then a bug somewhere.
@uschindler can you confirm? I vaguely recall hitting this one before, will try
to look into it too later this week.
6. I will help here. Give me a few (more!) days.
Awesome work dude, thanks very much. I'll leave the decision up to you
whether to get this PR merged or still work on it a bit more until it
stabilizes. I will look at items 5 and 6 very soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
