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

Allowing keys to be passed as a json file #113

Closed xavivars closed 6 years ago

xavivars commented 6 years ago

This feature will enable using apertium-apy installed with PIP and still use api-keys, as that feature will no longer depend on modifying a package's source file.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 945


Changes Missing Coverage Covered Lines Changed/Added Lines %
apertium_apy/handlers/translate.py 5 9 55.56%
apertium_apy/keys.py 7 19 36.84%
<!-- Total: 15 31 48.39% -->
Totals Coverage Status
Change from base Build 890: -0.2%
Covered Lines: 1056
Relevant Lines: 2106

💛 - Coveralls
sushain97 commented 6 years ago

This looks pretty good. One nit is, should it be class ApiKeys instead of class ApiKey since it holds multiple keys rather than a single one?

I wonder if rather than introducing a new dependency on commentjson, we could just copy over the couple relevant lines of code, specifically:

regex = r'\s*(#|\/{2}).*$'
regex_inline = r'(:?(?:\s)*([A-Za-z\d\.{}]*)|((?<=\").*\"),?)(?:\s)*(((#|(\/{2})).*)|)$'
lines = text.split('\n')

for index, line in enumerate(lines):
    if re.search(regex, line):
        if re.search(r'^' + regex, line, re.IGNORECASE):
            lines[index] = ""
        elif re.search(regex_inline, line):
            lines[index] = re.sub(regex_inline, r'\1', line)
xavivars commented 6 years ago

ApiKeys instead of ApiKey makes complete sense, I'll change that.

Regargind the other comment, I'm not super fan of copying libraries code into another program. Also, in this case, if commentsjson is not installed, the functionality still works (relying on the standard json library). But it's also true that the code needed is pretty small, so I'm OK with both approaches.

In any case, if we copy over the code, I think we should do the same thing with tests.

sushain97 commented 6 years ago

Also, in this case, if commentsjson is not installed, the functionality still works (relying on the standard json library).

Ah, good point. Okay, then I'm okay with keeping it.

sushain97 commented 6 years ago

Hmm, build seems to be failing?

xavivars commented 6 years ago

I know, but I don't understand why... there are almost no changes in the latest commit