-
Notifications
You must be signed in to change notification settings - Fork 3
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
Support for XML/HTML tags on the API calls and a myriad of other things #20
Support for XML/HTML tags on the API calls and a myriad of other things #20
Conversation
To avoid "permission denied"
…onfig-dir_permission 🐛 Fix default config directory permission to 0755
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…e#13) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ce#18) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ice#19) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Mostly English spelling and getting rid of deprecated Go functions.
The idea now is to rely more on `net/http` and less on our own internal table.
This essentially separates the actual API call from the translator bits, so we can now work on the extra nifty features we need.
New code requires passing structs representing JSON objects, instead of relying on loose interface conversions which may fail. Stricter is better!
Correction submitted by @coderabbitai Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
I had changed the text in main.g0, but forgot to update it in main_test.go
Not confirmed. But… this way we can be sure that it gets properly initialised and not scooped up into the “wrong” place…
…wynethLlewelyn/deepl-translate-cli into GwynethLlewelyn-adding-XML-tag-support
… at least, when called with the text formatting functions derived from the `fmt` package.
As suggested by @coderabbitai Also: return nil and not []string{}; we’re supposed to check for the `err` code, and nil is returned avoiding memory allocation of something that will never be used…
It’s more idiomatic that way, even if not necessarily easier to read (everybody knows their HTTP error codes by heart, right? no? well, then perhaps it’s better to follow the usual practice of Go’s core developers…)
The reason for it was that a potential JSON error was not being correctly caught; this was flagged by the test suite, and therefore I sort of fixed it. Now it correctly passes all tests it’s supposed to pass :)
@coderabbitai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Future TODO — have `Languages()` optionally reply in structured formats.
@Omochice it was actually fun to work with the AI to review the code — I even learned abut how to design some simple tests, too! I wonder if you need me to add anything else...? Cheers, |
I apologize for the delay 🙇 Thank you for your exceptional work! I've verified that it functions smoothly with both plain text and XML formats. Implementing subcommands is indeed a great idea, although it will necessitate a major version bump due to its impact on the user interface. Subcommands significantly enhance the code's extensibility, so I'll merge this pull request. Thanks again! |
Oops! Since this project was still in the v0.0.x release phase, there was no need to increase the major version. I'm thinking of releasing it as v0.1.0. |
…er things (#20) * 📝 Add badge to README.md * 📝 change usage text * 💪 Delete `stdin` flag * 📝 Update README * ⤴ Update version of library * 🥷 divide package * 🐛 fix test and ci * 💪 Add test * 🐛 Fix default config directory permission to 0755 To avoid "permission denied" * Add renovate.json (#11) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore: use conventional commit (#15) * Update module github.com/mattn/go-isatty to v0.0.19 (#12) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update module github.com/urfave/cli/v2 to v2.25.7 (#13) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update actions/checkout action to v4 (#16) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update actions/setup-go action to v4 (#17) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update goreleaser/goreleaser-action action to v5 (#18) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * build: update golang version * ci: update ci * ci: fix go-version * fix(deps): update module github.com/mattn/go-isatty to v0.0.20 (#19) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore: update .goreleaser * build: update go.sum * Chore: small fixes Mostly English spelling and getting rid of deprecated Go functions. * chore: refactor code for returning error message The idea now is to rely more on `net/http` and less on our own internal table. * chore: major code refactoring This essentially separates the actual API call from the translator bits, so we can now work on the extra nifty features we need. * feat: add simple function to return usage * feat: adding usage call * chore: refactor code to avoid object ambiguities New code requires passing structs representing JSON objects, instead of relying on loose interface conversions which may fail. Stricter is better! * chore: add help for languages * fix: add = to flag `type` usage line * feat: adding autocomplete files * doc: mentioned the autocompletion feature * fix: correctly display versions and build dates Note: I don’t know where the “builtBy” parameter comes from; currently, it needs to be force-pushed at bildtime with a -X tag to the linker. * chore: refactor more code, add option for glossary This was mostly meant as an experiment which can later be copied & pasted for other very similar options. apiCall() gained a new parameter, the method (because some things in the API stupidly use GET and not POST) * docs: better organise the explanatons, add links * chore: refactoring code — moving setup to “Before” * feat: add more flag support, upgrade dependencies * fix: revert changes: init must be done in main() I’ve attempted to do all initialisation chores under the “Before:” for the main cli.App loop. However, this wasn’t retrieving the data properly. Moving everything back to where it was in main(). * feat: major refactoring, we’ll get rid of settings In essence, we can use and reuse the DeepLClient type/object as the ‘de facto’ settings structure, we just need to find a way to read/save settings (possibly with cli-altsrv) * chore: adding ChatGPT-generated texts for testing * feat: adding support for (simple) debugging * test: test data in XML * docs: update README with latest changes * add readline package * fix: interactive prompt now works with readline * Update README.md Correction submitted by @coderabbitai Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: upgrade to latest versions yadda yadda * Bug: fix expected error text for DEEPL_TOKEN I had changed the text in main.g0, but forgot to update it in main_test.go * Docs: add backticks on comments * Docs: changes suggested by @coderabbitai * Docs: comments ending with period * Fix: match correct error text (changed on main.go) * Bug: possible scoping issues with deeplToken (?) Not confirmed. But… this way we can be sure that it gets properly initialised and not scooped up into the “wrong” place… * Chore: bump year to 2024 * Fix: add timeout as per @coderabbitai suggestion * Fix: add try-catch as per @coderabbitai suggestion * Chore: add test for Exists; err.Error() is redundant … at least, when called with the text formatting functions derived from the `fmt` package. * Docs: add comment * Fix: check for edge case of empty string As suggested by @coderabbitai Also: return nil and not []string{}; we’re supposed to check for the `err` code, and nil is returned avoiding memory allocation of something that will never be used… * Chore: use http.StatusXXX instead of numbers It’s more idiomatic that way, even if not necessarily easier to read (everybody knows their HTTP error codes by heart, right? no? well, then perhaps it’s better to follow the usual practice of Go’s core developers…) * Bug: missing `)` * Bug: fix a testing bug The reason for it was that a potential JSON error was not being correctly caught; this was flagged by the test suite, and therefore I sort of fixed it. Now it correctly passes all tests it’s supposed to pass :) * Doc: fix stupid typo * Docs: add comment made by @coderabbitai Future TODO — have `Languages()` optionally reply in structured formats. * Fix: error checking for writing configuration file Caught by @coderabbitai --------- Co-authored-by: mochi-MizLab <mochice.mls.ntl@gmail.com> Co-authored-by: Osamu Takiya <takiya@toran.sakura.ne.jp> Co-authored-by: Omochice <44566328+Omochice@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…er things (#20) * 📝 Add badge to README.md * 📝 change usage text * 💪 Delete `stdin` flag * 📝 Update README * ⤴ Update version of library * 🥷 divide package * 🐛 fix test and ci * 💪 Add test * 🐛 Fix default config directory permission to 0755 To avoid "permission denied" * Add renovate.json (#11) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore: use conventional commit (#15) * Update module github.com/mattn/go-isatty to v0.0.19 (#12) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update module github.com/urfave/cli/v2 to v2.25.7 (#13) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update actions/checkout action to v4 (#16) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update actions/setup-go action to v4 (#17) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update goreleaser/goreleaser-action action to v5 (#18) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * build: update golang version * ci: update ci * ci: fix go-version * fix(deps): update module github.com/mattn/go-isatty to v0.0.20 (#19) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore: update .goreleaser * build: update go.sum * Chore: small fixes Mostly English spelling and getting rid of deprecated Go functions. * chore: refactor code for returning error message The idea now is to rely more on `net/http` and less on our own internal table. * chore: major code refactoring This essentially separates the actual API call from the translator bits, so we can now work on the extra nifty features we need. * feat: add simple function to return usage * feat: adding usage call * chore: refactor code to avoid object ambiguities New code requires passing structs representing JSON objects, instead of relying on loose interface conversions which may fail. Stricter is better! * chore: add help for languages * fix: add = to flag `type` usage line * feat: adding autocomplete files * doc: mentioned the autocompletion feature * fix: correctly display versions and build dates Note: I don’t know where the “builtBy” parameter comes from; currently, it needs to be force-pushed at bildtime with a -X tag to the linker. * chore: refactor more code, add option for glossary This was mostly meant as an experiment which can later be copied & pasted for other very similar options. apiCall() gained a new parameter, the method (because some things in the API stupidly use GET and not POST) * docs: better organise the explanatons, add links * chore: refactoring code — moving setup to “Before” * feat: add more flag support, upgrade dependencies * fix: revert changes: init must be done in main() I’ve attempted to do all initialisation chores under the “Before:” for the main cli.App loop. However, this wasn’t retrieving the data properly. Moving everything back to where it was in main(). * feat: major refactoring, we’ll get rid of settings In essence, we can use and reuse the DeepLClient type/object as the ‘de facto’ settings structure, we just need to find a way to read/save settings (possibly with cli-altsrv) * chore: adding ChatGPT-generated texts for testing * feat: adding support for (simple) debugging * test: test data in XML * docs: update README with latest changes * add readline package * fix: interactive prompt now works with readline * Update README.md Correction submitted by @coderabbitai Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: upgrade to latest versions yadda yadda * Bug: fix expected error text for DEEPL_TOKEN I had changed the text in main.g0, but forgot to update it in main_test.go * Docs: add backticks on comments * Docs: changes suggested by @coderabbitai * Docs: comments ending with period * Fix: match correct error text (changed on main.go) * Bug: possible scoping issues with deeplToken (?) Not confirmed. But… this way we can be sure that it gets properly initialised and not scooped up into the “wrong” place… * Chore: bump year to 2024 * Fix: add timeout as per @coderabbitai suggestion * Fix: add try-catch as per @coderabbitai suggestion * Chore: add test for Exists; err.Error() is redundant … at least, when called with the text formatting functions derived from the `fmt` package. * Docs: add comment * Fix: check for edge case of empty string As suggested by @coderabbitai Also: return nil and not []string{}; we’re supposed to check for the `err` code, and nil is returned avoiding memory allocation of something that will never be used… * Chore: use http.StatusXXX instead of numbers It’s more idiomatic that way, even if not necessarily easier to read (everybody knows their HTTP error codes by heart, right? no? well, then perhaps it’s better to follow the usual practice of Go’s core developers…) * Bug: missing `)` * Bug: fix a testing bug The reason for it was that a potential JSON error was not being correctly caught; this was flagged by the test suite, and therefore I sort of fixed it. Now it correctly passes all tests it’s supposed to pass :) * Doc: fix stupid typo * Docs: add comment made by @coderabbitai Future TODO — have `Languages()` optionally reply in structured formats. * Fix: error checking for writing configuration file Caught by @coderabbitai --------- Co-authored-by: mochi-MizLab <mochice.mls.ntl@gmail.com> Co-authored-by: Osamu Takiya <takiya@toran.sakura.ne.jp> Co-authored-by: Omochice <44566328+Omochice@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
First of all, thank you so much for your code! It was really an inspiration for me.
As mentioned before, I've been playing around with many of the features of the DeepL API, starting with your code for inspiration — mostly because you already have a CLI framework in place!
Essentially, I needed access to the extra features that the DeepL API allows — more specifically, support for XML and HTML tags (which DeepL recognises as such, and is careful in translating only the text between the tags). This required a profound rewrite of the core of your code — which focused only on text translation — while still making it more-or-the-less compatible at the upper levels.
But this is by no means a "complete rewrite" of the code. For instance, I believe that it would be better to stick to a pure JSON implementation, both for the API requests and the replies; right now, the requests are still made as they originally were made in your code.
Your concept of saving some of the settings is an excellent idea! But it becomes harder — and requires a lot of code duplication — to support more settings. At some point, I sort of gave up maintaining it in sync with the extra settings; instead, most of the information in the settings subtly migrated to the context and to the "client" type (the client for the API calls, that is), except for the DeepL API key, which comes from the environment variables. There is a reason for that: I was checking on the CLI code, and they are currently beta-testing version 3 — which includes better support for saving the settings automatically! Currently, such support exists via cli-altsrc, but it requires quite a lot of code duplication. So, I'm sort of preparing myself for the upcoming changes, and possibly, in the future, I'll take a look at how it is implemented and see if it's straightforward.
Also, I'm very lazy in writing testing code 😞 I did update your existing testing code for the changes in function names and/or signatures, so that you can still run the tests; however, the additional code I've added does not have any testing — sorry about that!
Anyway, I hope you find at least some of it useful!
Summary by CodeRabbit
.gitignore
to exclude more files, improving developer workflow..editorconfig
file to ensure consistent coding styles.LICENSE
file.