[GitHub] jena pull request: JENA-1180: Add support for ComplexPhraseQueryPa...
Github user osma commented on the pull request: https://github.com/apache/jena/pull/146#issuecomment-219629095 I think the code is now good for merging. Thanks a lot! @afs Can I just merge the code to master, or is there some legal policy that must be followed for contributions like this? I've never merged PRs initiated by others. As for documentation, it lives outside this github repository (in the ASF SVN) and you can submit a patch via the web CMS - just click on "Improve this page" at the top of https://jena.apache.org/documentation/query/text-query.html We can close this PR after the code is merged, then resolve the JIRA issue after the documentation has been updated. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (JENA-1180) Add ComplexPhraseQueryParser to jena-text
[ https://issues.apache.org/jira/browse/JENA-1180?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15286110#comment-15286110 ] ASF GitHub Bot commented on JENA-1180: -- Github user osma commented on the pull request: https://github.com/apache/jena/pull/146#issuecomment-219629095 I think the code is now good for merging. Thanks a lot! @afs Can I just merge the code to master, or is there some legal policy that must be followed for contributions like this? I've never merged PRs initiated by others. As for documentation, it lives outside this github repository (in the ASF SVN) and you can submit a patch via the web CMS - just click on "Improve this page" at the top of https://jena.apache.org/documentation/query/text-query.html We can close this PR after the code is merged, then resolve the JIRA issue after the documentation has been updated. > Add ComplexPhraseQueryParser to jena-text > - > > Key: JENA-1180 > URL: https://issues.apache.org/jira/browse/JENA-1180 > Project: Apache Jena > Issue Type: Improvement > Components: Text >Affects Versions: Jena 3.1.0 >Reporter: Andrew Dolby >Priority: Minor > > Jena-text now supports alternative QueryParsers from JENA-1134, but is > missing ComplexPhraseQueryParser. This query parser can perform wildcard and > fuzzy queries on multiple terms within a phrase. > I'd like to add this parser following the same pattern used to add the > AnalyzingQueryParser in JENA-1134. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (JENA-1180) Add ComplexPhraseQueryParser to jena-text
[ https://issues.apache.org/jira/browse/JENA-1180?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15285860#comment-15285860 ] ASF GitHub Bot commented on JENA-1180: -- Github user adolby commented on the pull request: https://github.com/apache/jena/pull/146#issuecomment-219598981 Thank you for working on that refactoring! You're right, abstracting out makeSpec reduces the repetition for the ComplexPhraseQueryParser test. I've switched it over to inheriting from AbstractTestDatasetWithAnalyzer. Are there other changes you'd like to see in this PR? I'm not sure if/where documentation should be updated to reflect having the ComplexPhraseQueryParser as a QueryParser option. > Add ComplexPhraseQueryParser to jena-text > - > > Key: JENA-1180 > URL: https://issues.apache.org/jira/browse/JENA-1180 > Project: Apache Jena > Issue Type: Improvement > Components: Text >Affects Versions: Jena 3.1.0 >Reporter: Andrew Dolby >Priority: Minor > > Jena-text now supports alternative QueryParsers from JENA-1134, but is > missing ComplexPhraseQueryParser. This query parser can perform wildcard and > fuzzy queries on multiple terms within a phrase. > I'd like to add this parser following the same pattern used to add the > AnalyzingQueryParser in JENA-1134. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[GitHub] jena pull request: JENA-1180: Add support for ComplexPhraseQueryPa...
Github user adolby commented on the pull request: https://github.com/apache/jena/pull/146#issuecomment-219598981 Thank you for working on that refactoring! You're right, abstracting out makeSpec reduces the repetition for the ComplexPhraseQueryParser test. I've switched it over to inheriting from AbstractTestDatasetWithAnalyzer. Are there other changes you'd like to see in this PR? I'm not sure if/where documentation should be updated to reflect having the ComplexPhraseQueryParser as a QueryParser option. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: Updating dependency versions
Okay, coming back with a quick design question. The current type for authentication (o.a.j.atlas.web.auth.HttpAuthenticator), works via the following signature: public void apply(AbstractHttpClient client, HttpContext httpContext, URI target); The idea here (IIUC) is to apply authentication to an _extant_ HTTP client, context and URI. The problem for option 2 is that this is pretty late in the game, obviously later than client construction. Is the idea to bake in the HttpAuthenticator as part of the (immutable) state of a subclass of o.a.h.c.HttpClient that automatically applies the behavior at request execution? That tangles Jena and o.a.http.client types in a way that seems to me to be a bit odd, but my sense of Jena's idiom is pretty undeveloped. Just want to make sure I am on the right road. It would mean altering FormsAuthenticator to factor cookies out of the client into the context, and that means an HttpClientContext, so we'd end up with something like: public void apply(HttpClient client, HttpClientContext httpContext, URI target); which would break HttpAuthenticator impls out there in the world, but not horrifically. It would also mean that the client in that signature would have immutable state going forward, and that has more potential, I think, to break impls, but anyone doing a custom authenticator should also be able to factor that state out of the client into the context like I describe above. --- A. Soroka The University of Virginia Library > On May 15, 2016, at 8:33 AM, Andy Seaborne wrote: > > On 11/05/16 16:38, A. Soroka wrote: >> I recently did some work on a project for those exact deprecation >> warnings (introducing HttpClientBuilder). I would be happy to take a >> crack at those if you want to make me a ticket. I don't remember it >> being too hairy. > > https://issues.apache.org/jira/browse/JENA-576 > > The uprade looks easy but there is one area > > In the old API http-client, authentication is done by modifying the > HttpClient object. In the new API, HttpClient (CloseableHttpClient) is > immutable and chnages need to be made at the HttpClientBuilder stage. > > That impacts the HttpOp interface - some functions are of the form > HttpClient+HttpAuthenticator. And the public alls all go to one operation > that takes HttpClient+HttpAuthenticator. > > So I think (I have not tried) the forms: > > 1/ > operation(HttpClient, ) // No auth unless already in HttpClient > operation(HttpClientBuilder, HttpAuthenticator, ) > > i.e. HttpClientBuilder if HttpAuthenticator > > 2/ > Or move the creation/setup of authentication out of the operations call flow > and provide creator operations. > > Remove all > operation(HttpClient, HttpAuthenticator, ) > > and have > > HttpClient hc > > hc = createHttpClient(HttpAuthenticator, ) // default builder. > hc = createHttpClient(HttpClientBuilder, HttpAuthenticator, ) > > i.e. separate creation and auth setup. > > This, to me, looks more in tune with the new API > > 3/ > Stick to the deprecated AbstractHttpClient style. > > > ATM I favour (2). We don't preserve exact current HttpOp API in (1) or (2) > so let's do the better design. 3.2.0 if necessary (but I don't believe that > rigid semantic versions makes sense in large system with semi-independent > sub-systems; version number changes when many users aren't affected can be > more confusion than help; "semantic version" is a guidance not a rule). > >Andy
[GitHub] jena pull request: JENA-1180: Add support for ComplexPhraseQueryPa...
Github user osma commented on the pull request: https://github.com/apache/jena/pull/146#issuecomment-219381471 Thanks for the PR! The code looks good, as you say it's following the pattern established by JENA-1134. You are right about the unit tests - the inheritance hierarchy of analyzer-related unit tests is rather messy. There is a very long inheritance chain and e.g. TestDatasetWithAnalyzingQueryParser indirectly inherits from TestDatasetWithKeywordAnalyzer. There is actually a sort of logic here, because the tests specified in TestDatasetWithKeywordAnalyzer will also be run on the subclasses, as the analyzer configurations are compatible. This gives us some unit tests "for free". I did some reorganizing of the test classes, namely, I abstracted out AbstractTestDatasetWithAnalyzer from TestDatasetWithKeywordAnalyzer (see https://github.com/apache/jena/commit/8a20f238ffc5aed23648b038132fe74cbca2c85c). Unless I'm missing ssomething, you should now be able to inherit from AbstractTestDatasetWithAnalyzer and avoid doing the full makeSpec in your test class. Would you mind switching over to that? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
