Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update index.js to allow for multiple URL support #7

Merged
merged 6 commits into from
Mar 18, 2019
Merged

Update index.js to allow for multiple URL support #7

merged 6 commits into from
Mar 18, 2019

Conversation

rawr51919
Copy link

@rawr51919 rawr51919 commented Mar 6, 2019

WIP update to index.js to allow for multiple URL support in the API by adding a new argument field (ties into PR vitalets/google-translate-token#2). This fixes matheuss#60, matheuss/google-translate-token#10 and part of issue #4.

WIP update to index.js to allow for multiple URL support in the API by adding a new argument field (ties into PR vitalets/google-translate-token#2).
@vitalets
Copy link
Owner

vitalets commented Mar 7, 2019

Could you add a unit test to ensure that it works:

test('translate via custom tld', async t => {
    try {
        const res = await translate('vertaler', {tld: 'cn'});

        t.is(res.text, 'translator');
        t.false(res.from.language.didYouMean);
        t.is(res.from.language.iso, 'nl');
        t.false(res.from.text.autoCorrected);
        t.is(res.from.text.value, '');
        t.false(res.from.text.didYouMean);
    } catch (err) {
        t.fail(err.code);
    }
});

@rawr51919
Copy link
Author

@vitalets I'll add that in, just I need some pointing in the right direction on what exactly should be done to finish the implementation of this extra field.

Updates test.js to add support for testing the WIP custom field
@rawr51919
Copy link
Author

rawr51919 commented Mar 7, 2019

Done. Added the test in, just now I need to figure out how I'm gonna actually implement that extra field. (It's just using com as a default value for now until I can figure out how I should go about implementing it, and what exactly should be done if an invalid argument is passed to it (say, con).

@vitalets
Copy link
Owner

I think you should add default com value similar to existing defaults:

opts.tld = opts.tls || 'com';

and use it when building url:

var url = 'https://translate.google.' + opts.tld + '/translate_a/single';

Revise the index.js URL code to help with the development of the custom URL argument
@codecov-io
Copy link

codecov-io commented Mar 12, 2019

Codecov Report

Merging #7 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage    92.3%   92.42%   +0.11%     
==========================================
  Files           2        2              
  Lines          65       66       +1     
==========================================
+ Hits           60       61       +1     
  Misses          5        5
Impacted Files Coverage Δ
index.js 90.19% <100%> (+0.19%) ⬆️
languages.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3cb689...1bfad3b. Read the comment docs.

@rawr51919
Copy link
Author

rawr51919 commented Mar 12, 2019

That's done. Will this allow for the extra URL argument to be passed in and be used in translation (it'll still need work on the token side of things to get the token from the URL passed here)?

Updates index.js again to actually make the field usable (I think)
Reverts previous commit as I'm unsure how this is supposed to work
index.js Outdated Show resolved Hide resolved
Updates index.js to add in vitalets' suggestion (allows for the tld variable to be passed to the token side of things)
@rawr51919
Copy link
Author

@vitalets Does this look any better codewise? I've done what you needed me to do here.

@vitalets vitalets merged commit 5a242d4 into vitalets:master Mar 18, 2019
vitalets pushed a commit that referenced this pull request Mar 18, 2019
@vitalets
Copy link
Owner

I'v cloned your branch made some fix and merged. Thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

China doesn't work: Default URL banned, no proxy support, alternate URL not working
3 participants