Added support for Dictionaries#10
Conversation
|
Hello @Richard-Degenne, Nice to meet you! Thanks for submitting a PR. I will look through it in a couple of days. |
|
Hi @andhapp, I added support for another endpoint of the dictionary API. |
|
Any input on the current state of this PR? |
|
Sorry man! It's been failing on Travis. I will have a look at it tonight. It must be something in the setup. I have glanced through the PR and it seems okay. Will look through it again just to make sure. Thanks for your patience! |
|
Yes, the Travis error seems unrelated, but I can't say for sure.
No worries. :) |
README.md
Outdated
| #### Adding entries to a dictionary | ||
|
|
||
| ``` | ||
| entry = TextRazor::DictionaryEntry(id: 'my-entry', text: 'Text to be matched') |
There was a problem hiding this comment.
DictionaryEntry and Dictionary should be internal classes, IMHO. The API interface should be something like this:
client = TextRazor::Client.new('api_key')
client.create_dictionary('my-dictionary')
# This can have keyword arguments but I'd not do it if it's not anywhere else in the library, makes it inconsistent
client.create_dictionary_entries('my-dictionary', {id: 'my-entry', text: 'Text to be matched'})
These method calls create_dictionary and create_dictionary_entries should return the actual instances of Dictionary and DictionaryEntry but there is no need for the consumer to know how to create these instances. client hides that behind a facade and for a good reason.
There was a problem hiding this comment.
Agreed. I'll rework these parts.
lib/textrazor/api_client.rb
Outdated
| @@ -0,0 +1,36 @@ | |||
| require 'rest_client' | |||
There was a problem hiding this comment.
What's the main reason to separate this out into an api_client class? `
There was a problem hiding this comment.
Agreed. I'll rework this part.
lib/textrazor/request.rb
Outdated
|
|
||
| def self.build_query(text, options) | ||
| query = {"text" => text, "apiKey" => options.delete(:api_key)} | ||
| query = {"text" => text} |
There was a problem hiding this comment.
Removing it from here means this is not duplicated across 4 or 5 different methods. Any particular reason to do this?
There was a problem hiding this comment.
The API key must be passed as a x-textrazor-key header.
As of today, this is not an issue because the only endpoint supported by the gem is a POST request, but adding support for dictionaries means supporting GET, PUT and DELETE requests.
|
|
||
| module TextRazor | ||
|
|
||
| class ApiResponse |
There was a problem hiding this comment.
Similar to ApiRequest, Response class was meant to take care of this responsibility. Any particular reason to do this.
There was a problem hiding this comment.
The Response class is heavily oriented towards parsing responses to analysis requests.
The more general ApiResponse can handle any kind of response from TextRazor. (Side note: Response inherits from ApiResponse)
|
Hey @Richard-Degenne, I had a look at the PR and left some comments on it. |
|
Thanks for the review! I'm a bit snowed under at the moment, but I'll look into your remarks as soon as I have a moment. |
|
Hi @andhapp, It's been a while, but I intend to work on this PR in the coming days. I'll answer your reviews and rework some things here and there. |
Richard-Degenne
force-pushed
the
dictionaries
branch
from
c818ca2 to
9f11997
Compare
|
Hi @andhapp, I rebased the branch onto |
|
Thanks. I've reviewed your other PR. Will look at this one next. |
Richard-Degenne
force-pushed
the
dictionaries
branch
from
9f11997 to
07dcd06
Compare
|
Hi @andhapp, Since the other PR got merged (thanks for that, btw), I rebased this one onto |
|
Hi @andhapp, Any input on the current state of this PR? :) |
|
Hi @andhapp, Allow to bump. I understand you have some problems with GitHub notifications, so maybe this one will get through. 😛 |
2 participants
This PR adds basic support for TextRazor's dictionary features.
It adds 4 new endpoints to the API :
The PR also revamps the way requests are handled, since until now, only the analysis API was considered.