apertium / apertium-apy

📦 Apertium HTTP Server in Python
https://wiki.apertium.org/wiki/Apertium-apy
GNU General Public License v3.0
32 stars 42 forks source link

Added Support for handling multiple 'q' parameters #105

Closed Axle7XStriker closed 2 years ago

Axle7XStriker commented 6 years ago

Currently, APy doesn't support multiple q's as parameters when given for translation. I have resolved this by storing multiple responses(local) first and then sending all at once in a global response. Each parameter q is iteratively translated by a given langpair and an error is raised if at least one of the translation fails.

refs #86

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 886


Changes Missing Coverage Covered Lines Changed/Added Lines %
apertium_apy/handlers/translate_raw.py 0 8 0.0%
<!-- Total: 16 24 66.67% -->
Totals Coverage Status
Change from base Build 884: 0.1%
Covered Lines: 1054
Relevant Lines: 2095

💛 - Coveralls
Axle7XStriker commented 6 years ago

One of the problems which were there in the previous PR was how the response was structured as I was sending the whole local response in the responseData field instead of just translatedText field. The other problem was the auto_finish problem. In it, tornado automatically invokes the finish() whenever it gets the response even if its a local response. Although I still don't clearly understand how the yield is working in the code and I am also not sure if my handling of the finish() is correct or not.

sushain97 commented 6 years ago

When you do foo = yield bar it gets stored like you want it. When you do yield bar it's like a return. You might have to do some magic to allow all the translations to occur at the same time. In JS, the equivalent would be, Promise.all. There's certainly some equivalent mechanism we can use here.

Axle7XStriker commented 6 years ago

Finally, I was able to find the equivalent which you were referring to. I found out that tornado provides a similar functionality in the form of gen.multi_future() which does the same thing as Promise.all. I also found out the reason behind the finish() being called twice, that was because before sending the final response I didn't check, whether the response is empty (because of certain error in resolving language pair) or filled previously but now that is also resolved with this commit.

sushain97 commented 6 years ago

Also, please add a test that verifies multiple q params works correctly. It should just be 3-5 LOC.

Axle7XStriker commented 6 years ago

There is an Incompatible type assignment error which is coming on running mypy, so to fix this I think I should use the send_response function in the if condition only but the thing I don't get is, in the previous commits also I had done the same thing only but it passed there so why isn't it passing now? Also, for the test case, I will open a new PR as soon as I am done with it.

sushain97 commented 6 years ago

What exactly is the type error? Also, tests should go in the same PR so I can verify that they pass.

Axle7XStriker commented 6 years ago

By type error, I mean that in line 181, I am assigning a dict to a variable which contains a list on which an error is raised by mypy. But this error was not raised in Fixed twice Finish() call commit which also had the same line of code.

sushain97 commented 6 years ago

Yeah, I think mypy is being a bit trigger happy here. I suggest just using an inline ternary. i.e.

'responseData': response if len(response) > 1 else response[0]['responseData']

I'm not a huge fan of variables changing type anyway.

sushain97 commented 6 years ago

@unhammer see any issues?

unhammer commented 6 years ago

What about subclass users of translate_and_respond?

sushain97 commented 6 years ago

Good point, missed that. Looks like translate_raw is the only subclass user ( and we don't have tests for it :( ). I think that we should have a new @property in the base handler along the lines of mark_unknown and replicate the functionality in translate_raw since aside from the mark_unknown = ... code, the rest is pretty standard.

Axle7XStriker commented 6 years ago

Although, I get that we need to replicate the functionality in translate_raw, but I don't get specifying the @property for mark_unknown in the base handler. Can you please explain that.

sushain97 commented 6 years ago

I don't get specifying the @property for mark_unknown in the base handler. Can you please explain that.

Create a method in the base translation handler called mark_unknown that returns a boolean. Then, use the @property decorator on it so that it can be referred to as self.mark_unknown (i.e. without the parens).