Issue 29886648: Issue #6942 - Add XTM integration in CMS (Closed)
|
Created:
Sept. 20, 2018, 4:24 p.m. by Tudor Avram Modified:
Oct. 10, 2018, 3:09 p.m. Visibility:
Public. |
DescriptionIssue #6942 - Add XTM integration in CMS
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressed initial comments #
Total comments: 99
Patch Set 3 : Addressed comments on patch set #2 #
Total comments: 6
Patch Set 4 : Addressed comments from patch set #3 #
Total comments: 6
Patch Set 5 : Addressed comments from Patch Set #4 #
Total comments: 7
MessagesTotal messages: 12
Hi Vasily and Winsley, Here is my code for the XTM integration into CMS. There still are a couple of TODOs in `cms/bin/xtm_translations/translate_xtm_cloud.py`, but those are just because I was not sure what some arguments (eg: project description) would mean on our end or if they'd have any meaning at all. Thanks, Tudor.
Hey Tudor! These are my comments so far. Don't be surprised that they are kind of in different files, I was just looking over stuff to understand the logic better and commented right away. Cheers, Vasily https://codereview.adblockplus.org/29886648/diff/29886649/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29886648/diff/29886649/.gitignore#newcode9 .gitignore:9: .idea/ You should also add this to .hgignore to keep them in sync. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... File cms/bin/xtm_translations/constants.py (right): https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:15: """Messages and other constants used by the translation script.""" Nit: The docstring is more noticeable if you leave an empty line before it. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:18: def input_fn(text): Could this be a better fit in `utils.py`? https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:64: EXISTENT_PROJECT = ('Error! There already exists an XTM project associated' What do you think about renaming this to PROJECT_EXISTS? Seem more in line with the rest and kind of easier to understand. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:76: UNSUPPORTED_LOCALE = ('The following locale is not supported: "{0}". ' If you agree about EXISTENT_PROJECT -> PROJECT_EXISTS, this can be called LOCALE_NOT_SUPPORTED. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:122: CONFIG = { Perhaps this and some previous bundles of constants could also be namespa... err, I mean classes instead :D In the code where these things are used it's easier to see what's going on when you see something like cnts.CONFIG.XTM_SECTION compared to cnts.CONFIG['XTM_section'] and also some typing is saved. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... File cms/bin/xtm_translations/projects_handler.py (right): https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:21: import cms.bin.xtm_translations.constants as cnts What do you think about using "cnst" instead of "cnts" as the abbreviation? When I read "cnts" it seems to be "counts" with vowels removed whereas "cnst" would be more like "const" with vowels removed (and we actually don't even have to remove the vowels, "const" is not a reserved word in Python). Does this make sense? https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:30: input = cnts.input_fn Since this is not used this much, we could just call it directly as `cnts.input_fn()`. Actually, is it used at all in this file? https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... File cms/bin/xtm_translations/translate_xtm_cloud.py (right): https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/translate_xtm_cloud.py:15: """Script handling interaction with the XTM Cloud REST API.""" This part would be more readable if we separate the docstring by empty lines from the comment above and from the imports below. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/translate_xtm_cloud.py:31: input = cnts.input_fn Again, we can just call `cnts.input_fn` (or maybe call it `cnts.input`), I don't think it pays off to assign it to a global variable. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... File cms/bin/xtm_translations/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:147: try: We can use a `collections.defaultdict(OrderedDict)` as the value of `page_strings` to replace this try-except! https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:241: if xtm_locale not in cnts.SUPPORTED_LOCALES: I think this if statement would feel more natural if you remove the `not` and swap the branches.
Hi Vasily, I addressed your initial comments. Let me know what you think. Thanks, Tudor. https://codereview.adblockplus.org/29886648/diff/29886649/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29886648/diff/29886649/.gitignore#newcode9 .gitignore:9: .idea/ On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > You should also add this to .hgignore to keep them in sync. Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... File cms/bin/xtm_translations/constants.py (right): https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:15: """Messages and other constants used by the translation script.""" On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > Nit: The docstring is more noticeable if you leave an empty line before it. Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:18: def input_fn(text): On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > Could this be a better fit in `utils.py`? Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:64: EXISTENT_PROJECT = ('Error! There already exists an XTM project associated' On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > What do you think about renaming this to PROJECT_EXISTS? Seem more in line with > the rest and kind of easier to understand. Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:76: UNSUPPORTED_LOCALE = ('The following locale is not supported: "{0}". ' On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > If you agree about EXISTENT_PROJECT -> PROJECT_EXISTS, this can be called > LOCALE_NOT_SUPPORTED. Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:122: CONFIG = { On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > Perhaps this and some previous bundles of constants could also be namespa... > err, I mean classes instead :D > > In the code where these things are used it's easier to see what's going on when > you see something like cnts.CONFIG.XTM_SECTION compared to > cnts.CONFIG['XTM_section'] and also some typing is saved. Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... File cms/bin/xtm_translations/projects_handler.py (right): https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:21: import cms.bin.xtm_translations.constants as cnts On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > What do you think about using "cnst" instead of "cnts" as the abbreviation? When > I read "cnts" it seems to be "counts" with vowels removed whereas "cnst" would > be more like "const" with vowels removed (and we actually don't even have to > remove the vowels, "const" is not a reserved word in Python). Does this make > sense? Used `const`. And replaced it in all other files Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:30: input = cnts.input_fn On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > Since this is not used this much, we could just call it directly as > `cnts.input_fn()`. > > Actually, is it used at all in this file? Oh, actually it isn't anymore. It was used initially, when I was asking the user if they wanted to continue creating a project if there already was one (and thus overwriting the original entry in `settings.ini`), but I removed that piece of logic. Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... File cms/bin/xtm_translations/translate_xtm_cloud.py (right): https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/translate_xtm_cloud.py:15: """Script handling interaction with the XTM Cloud REST API.""" On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > This part would be more readable if we separate the docstring by empty lines > from the comment above and from the imports below. Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/translate_xtm_cloud.py:31: input = cnts.input_fn On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > Again, we can just call `cnts.input_fn` (or maybe call it `cnts.input`), I don't > think it pays off to assign it to a global variable. Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... File cms/bin/xtm_translations/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:147: try: On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > We can use a `collections.defaultdict(OrderedDict)` as the value of > `page_strings` to replace this try-except! Done. https://codereview.adblockplus.org/29886648/diff/29886649/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:241: if xtm_locale not in cnts.SUPPORTED_LOCALES: On 2018/09/24 16:32:11, Vasily Kuznetsov wrote: > I think this if statement would feel more natural if you remove the `not` and > swap the branches. Done.
Hi Tudor, I have gone through everything in cms/bin/xtm_translations except for xtm_api.py. Below are my comments so far. The comments are mostly about naming and structure. I think those are quite important and would make the code easier to read and maintain, but can also be kind of arbitrary at times. If something doesn't make sense to you, let's discuss. I'll look at the rest asap and give you more ;) Cheers, Vasily https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/__main__.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/__main__.py:1: from cms.bin.xtm_translations.translate_xtm_cloud import main Most of the code of xtm_translations doesn't need to live in cms/bin (which is a directory for scripts). Perhaps a better approach would be to move everything but this file into cms/xtm_translations (maybe even better cms/translations/xtm, just in case we want to have multiple backends at some point) and this file could become cms/bin/xtm_translations.py or have any other name that is appropriate for the CLI frontend. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/constants.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:157: FILENAMES = { Maybe this could also be a namespace? The items inside seem more like two separate constants rather than a mapping. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/projects_handler.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:15: import sys Nit: it's nice to have an empty line after the top comment. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:120: new_jobs = utils.run_and_wait( What if this fails after all the retries? Above you catch the exception and do sys.exit() -- is there a reason to do it differently here? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:174: valid_locales = utils.get_locales(locales_path, default_locale) but source.list_locales()? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:192: def main(args): Maybe this function would fit better in translate_xtm_cloud.py -- it's basically doing routing anyway and there it would be easier to see what's in args.projects_func, so it would be easier to read it. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:26: __all__ = [ I have an impression that some of these functions would sit better in projects_handler, next to the other code that deals with getting the strings out of the website and syncing them with the XTM project. For example things like extract_strings(), resolve_locales(), map_locales(), etc. read_token() seems quite XTM API specific, or maybe it's arguments handling... And then there are more general functions like log_resulting_jobs() and run_and_wait() that totally make sense in this module. Do you think it would make sense to move the project/website-related functions to project_handler or do you like the layout as it is more? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:94: def resolve_naming_conflicts(name): It seems like this doesn't really handle name conflicts but instead does something like sanitizing. Maybe a better name would be sanitize_project_name()? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:119: return valid_name[:const.ProjectName.MAX_LENGTH] I would just always do this and remove the if. It seems pretty clear that we're just truncating the string here and Python doesn't mind a no-op truncation. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:248: def get_local_files(page_strings): This function does 3 things: - select pages that have any translation strings, - add json extension to them to get local filenames, - map them to remote names checking that none are too long I think bundling them together is a bit confusing and is not a very easy to understand abstraction. This forces the person who looks at projects_handler.py to go here and look at the code understand what's happening inside of the function (the docstring also seems to not be clear enough). What do you think about the following plan? - select pages that have any strings outside of this function, - rename this function to something like make_filename_map(page_names) and have it do the other two parts. I see that this function is called twice in project_handler, so moving things out of it seems not so good but actually the two surrounding lines are the same in both places so I have the following idea for this: what if get_files_to_upload had a signature: source -> list[(remote_name, string_dict)] instead? Both create_project and upload_files could just call it on the source to get the data structure that they pass to the API. Does this make sense? Do you agree that the resulting code structure is easier to understand? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:258: set This data structure looks kind of like it wants to be a dict. Why did you go for a set of tuples instead? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:282: def resolve_remote_filename(filename, source_dir, locales): Maybe a better name would be to_local_filename() or remote_to_local() or something like that? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:330: wait: int Maybe this could be called "retry_delay" for more clarity. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:332: exp: Exception I think a better name for exception is exc, exp sounds more like "expression". https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:338: max_tries: int Idea: You can replace this and the following parameter with one parameter: retries, that will be reduced by one with each recursive call. And when it reaches 0 we don't retry anymore. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:368: """Delete files with an extension from the tree starting with dir_path. It seems like this function is about deleting translation files, and we even mention locales in the argument list. If I'm right, maybe it should explicitly say that? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:388: def get_locales(path, default): Why don't we use source.list_locales() for this? We should have the source around where this is called, right? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:414: def write_to_file(data, file_path): I wonder if it makes sense to use a similar function from cms.utils for this. What do you think? https://codereview.adblockplus.org/29886648/diff/29891625/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/sources.py#newc... cms/sources.py:278: config.write(open(self.get_path('settings.ini'), 'w')) You need to close the file here. I don't think config.write() will close it. A nice way to do it would be: with open(...) as conffile: config.write(conffile)
Hi Tudor, I've finished going through everything. Here are some more comments! Cheers, Vasily https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/xtm_api.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:18: import requests Imports order should be stdlib, 3rd party, local with an empty line between them. Requests is 3rd party. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:25: _BASE_URL = 'https://wstest2.xtm-intl.com/rest-api/' Shouldn't this URL be configurable somehow? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:46: super(XTMCloudException, self).__init__(full_message) Do you think somebody might be interested in having the code, message and action as separate attributes on this exception for inspection? Maybe this is premature, but in principle we could assign them to attributes... https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:53: 'create': 'projects', So I think I understand my biggest problem with having these dicts of constants like this. Usually we use constants to avoid magical numbers or strings. So that instead of foo.bar("create") we have foo.bar(CREATE) and then people reading the code are not wondering what this string means. But when we use dicts for storing constants we kind of end up with other magical strings. For example, instead of: url = self.base_url + 'projects' we now have: url = self.base_url + self._URL_PATHS['create'] But then it seems we have a magical string here again. Do you think it would make sense to convert these dicts to "namespaces" too? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:60: _DEFAULT_CONTENT_TYPE = 'multipart/form-data' Are we using this constant anywhere? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:90: """Private method executing requests and returning the result. Nit: I think we don't really need to include the information that is already obvious from the method name. Usually the best docstring for a method is something like """<verb> [<noun phrase>] ...""" and here it could be just """Send request to the API and return the response.""" https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:276: 'matchType': self._MATCH_TYPE['update'] if overwrite else Maybe it would make more sense to have True and False as the keys of this dict so that you could just index it with `overwrite`. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:399: def get_token(username, password, user_id): Maybe this should also be a method of XTMCloudAPI class? https://codereview.adblockplus.org/29886648/diff/29891625/docs/usage/README.md File docs/usage/README.md (right): https://codereview.adblockplus.org/29886648/diff/29891625/docs/usage/README.m... docs/usage/README.md:1: #XTM Integration This file should probably not be called README.md but something more specific line `xml-sync.md` and also linked from the main README. https://codereview.adblockplus.org/29886648/diff/29891625/docs/usage/README.m... docs/usage/README.md:1: #XTM Integration Not having space between # and XTM doesn't work with Grip (https://github.com/joeyespo/grip), which I think uses github renderer, so it might be better to put it there just to be on the safe side. https://codereview.adblockplus.org/29886648/diff/29891625/docs/usage/README.m... docs/usage/README.md:5: ```commandline You can also introduce code blocks like this: Some text text text: code, indented by 4 spaces some more code This reads better when you're looking at plaintext. Is there an advantage to backquotes and commandline type? https://codereview.adblockplus.org/29886648/diff/29891625/tests/conftest.py File tests/conftest.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/conftest.py#n... tests/conftest.py:12: from tests.xtm_mock_api import get_configured_app Perhaps it would be better to import this as get_xtm_app or maybe just from test import xtm_mock_api and then use it with a dot. On the other hand, if you move this code elsewhere (see comment below), maybe there it will be all about XTM stuff and this import would be fine. https://codereview.adblockplus.org/29886648/diff/29891625/tests/conftest.py#n... tests/conftest.py:30: @pytest.fixture(scope='session') It seems that all this new stuff is just for testing XTM integration so maybe it would fit better into the file with that test suite. Conftest is supposed to be more for general stuff that gets used all over the place. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_site/set... File tests/test_site/settings.ini (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_site/set... tests/test_site/settings.ini:17: [XTM] Do we need this section here, if it's empty anyway? https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_api.py File tests/test_xtm_api.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_api.... tests/test_xtm_api.py:15: """Tests for the XTM API integration.""" Nit: empty lines https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_api.... tests/test_xtm_api.py:58: """Test if the creation of files behaves appropriately.""" Nit: this could be just "Test creation of files" -- seems like we haven't lost any information? If you agree, the other doctstrings could be also shortified in the same way. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... File tests/test_xtm_translate.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translate.py:15: from __future__ import unicode_literals Nit: you know https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translate.py:17: import pytest Nit: pytest should be after all of that stdlib stuff. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... File tests/test_xtm_translations_utils.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translations_utils.py:15: import pytest Nit: pytest should be after stdlib. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translations_utils.py:32: """""" Nit: probably no sense in having an empty docstring. But maybe here some explanation would help. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translations_utils.py:110: exp_out = os.path.join(str(temp_site), *['foo', 'bar', 'faz', 'test.json']) Isn't this kind of the same as just removing the star and the square brackets? https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translations_utils.py:189: api.add_target_languages(_PROJECT_ID, ['ro_RO']) ;) https://codereview.adblockplus.org/29886648/diff/29891625/tests/utils.py File tests/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/utils.py#newc... tests/utils.py:23: import pytest Nit: there should be an empty line before pytest, which is not stdlib. https://codereview.adblockplus.org/29886648/diff/29891625/tests/xtm_mock_api.py File tests/xtm_mock_api.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/xtm_mock_api.... tests/xtm_mock_api.py:60: auth = request.headers.get('Authorization') Can this just be `request.headers['Authorization']`? https://codereview.adblockplus.org/29886648/diff/29891625/tests/xtm_mock_api.... tests/xtm_mock_api.py:165: }for tl in xtm_mock.config['target_languages']] Nit: space before "for"? https://codereview.adblockplus.org/29886648/diff/29891625/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29886648/diff/29891625/tox.ini#newcode21 tox.ini:21: httplib2 Do we need httplib2 in addition to requests? Sorry if we've discussed it and I forgot :/
Hi Vasily, I addressed your comments from the previous patch (except for a couple of them, such as making `get_token()` a method of `XTMCloudAPI`. I know we discussed this in person, but I still feel more like it doesn't belong in there). Also, waiting for @wspee to schedule a call to discuss some other details. Thanks, Tudor. Thanks, Tudor. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/__main__.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/__main__.py:1: from cms.bin.xtm_translations.translate_xtm_cloud import main On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > Most of the code of xtm_translations doesn't need to live in cms/bin (which is a > directory for scripts). Perhaps a better approach would be to move everything > but this file into cms/xtm_translations (maybe even better cms/translations/xtm, > just in case we want to have multiple backends at some point) and this file > could become cms/bin/xtm_translations.py or have any other name that is > appropriate for the CLI frontend. We discussed this in person and, as far as I remember, we decided to do this in a separate ticket? https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/constants.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/constants.py:157: FILENAMES = { On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > Maybe this could also be a namespace? The items inside seem more like two > separate constants rather than a mapping. Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/projects_handler.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:15: import sys On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > Nit: it's nice to have an empty line after the top comment. Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:120: new_jobs = utils.run_and_wait( On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > What if this fails after all the retries? Above you catch the exception and do > sys.exit() -- is there a reason to do it differently here? Nope. I just forgot to add that here :D https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:120: new_jobs = utils.run_and_wait( On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > What if this fails after all the retries? Above you catch the exception and do > sys.exit() -- is there a reason to do it differently here? Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:174: valid_locales = utils.get_locales(locales_path, default_locale) On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > but source.list_locales()? As we discussed in person, I wrote that method because `source.list_locales()` ignores locales for which the corresponding directory is empty. And I think that would be the case especially for cases when we want to add new languages to a website. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/projects_handler.py:192: def main(args): On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > Maybe this function would fit better in translate_xtm_cloud.py -- it's basically > doing routing anyway and there it would be easier to see what's in > args.projects_func, so it would be easier to read it. Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:26: __all__ = [ On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > I have an impression that some of these functions would sit better in > projects_handler, next to the other code that deals with getting the strings out > of the website and syncing them with the XTM project. For example things like > extract_strings(), resolve_locales(), map_locales(), etc. > > read_token() seems quite XTM API specific, or maybe it's arguments handling... > > And then there are more general functions like log_resulting_jobs() and > run_and_wait() that totally make sense in this module. > > Do you think it would make sense to move the project/website-related functions > to project_handler or do you like the layout as it is more? The idea of this module was to have all the small and more general functions that actually are XTM-specific. But I'm happy to other suggestions, if you have some. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:94: def resolve_naming_conflicts(name): On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > It seems like this doesn't really handle name conflicts but instead does > something like sanitizing. Maybe a better name would be sanitize_project_name()? Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:119: return valid_name[:const.ProjectName.MAX_LENGTH] On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > I would just always do this and remove the if. It seems pretty clear that we're > just truncating the string here and Python doesn't mind a no-op truncation. Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:248: def get_local_files(page_strings): On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > This function does 3 things: > - select pages that have any translation strings, > - add json extension to them to get local filenames, > - map them to remote names checking that none are too long > > I think bundling them together is a bit confusing and is not a very easy to > understand abstraction. This forces the person who looks at projects_handler.py > to go here and look at the code understand what's happening inside of the > function (the docstring also seems to not be clear enough). > > What do you think about the following plan? > - select pages that have any strings outside of this function, > - rename this function to something like make_filename_map(page_names) and have > it do the other two parts. > > I see that this function is called twice in project_handler, so moving things > out of it seems not so good but actually the two surrounding lines are the same > in both places so I have the following idea for this: what if > get_files_to_upload had a signature: source -> list[(remote_name, string_dict)] > instead? Both create_project and upload_files could just call it on the source > to get the data structure that they pass to the API. > > Does this make sense? Do you agree that the resulting code structure is easier > to understand? Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:258: set On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > This data structure looks kind of like it wants to be a dict. Why did you go for > a set of tuples instead? Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:282: def resolve_remote_filename(filename, source_dir, locales): On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > Maybe a better name would be to_local_filename() or remote_to_local() or > something like that? Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:330: wait: int On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > Maybe this could be called "retry_delay" for more clarity. Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:332: exp: Exception On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > I think a better name for exception is exc, exp sounds more like "expression". Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:338: max_tries: int On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > Idea: You can replace this and the following parameter with one parameter: > retries, that will be reduced by one with each recursive call. And when it > reaches 0 we don't retry anymore. Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:368: """Delete files with an extension from the tree starting with dir_path. On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > It seems like this function is about deleting translation files, and we even > mention locales in the argument list. If I'm right, maybe it should explicitly > say that? Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:388: def get_locales(path, default): On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > Why don't we use source.list_locales() for this? We should have the source > around where this is called, right? As discussed in person, I need to see all the locales (even the ones where the directories are empty and which source.list_locales() ignores) https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:414: def write_to_file(data, file_path): On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > I wonder if it makes sense to use a similar function from cms.utils for this. > What do you think? As discussed in person, there's no such function in `cms.utils`. When doing refactoring, we might want to put this there and re-use it from `generate_static_pages.py`, but that would have to be a separate ticket. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/xtm_api.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:18: import requests On 2018/09/26 15:45:24, Vasily Kuznetsov wrote: > Imports order should be stdlib, 3rd party, local with an empty line between > them. Requests is 3rd party. Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:25: _BASE_URL = 'https://wstest2.xtm-intl.com/rest-api/' On 2018/09/26 15:45:24, Vasily Kuznetsov wrote: > Shouldn't this URL be configurable somehow? Going to discuss the level of configurability with Winsley later this week. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:46: super(XTMCloudException, self).__init__(full_message) On 2018/09/26 15:45:24, Vasily Kuznetsov wrote: > Do you think somebody might be interested in having the code, message and action > as separate attributes on this exception for inspection? Maybe this is > premature, but in principle we could assign them to attributes... Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:53: 'create': 'projects', On 2018/09/26 15:45:24, Vasily Kuznetsov wrote: > So I think I understand my biggest problem with having these dicts of constants > like this. Usually we use constants to avoid magical numbers or strings. So that > instead of foo.bar("create") we have foo.bar(CREATE) and then people reading the > code are not wondering what this string means. > > But when we use dicts for storing constants we kind of end up with other magical > strings. For example, instead of: > > url = self.base_url + 'projects' > > we now have: > > url = self.base_url + self._URL_PATHS['create'] > > But then it seems we have a magical string here again. Do you think it would > make sense to convert these dicts to "namespaces" too? Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:60: _DEFAULT_CONTENT_TYPE = 'multipart/form-data' On 2018/09/26 15:45:24, Vasily Kuznetsov wrote: > Are we using this constant anywhere? Nope. Was using that initially, forgot to remove id :D Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:90: """Private method executing requests and returning the result. On 2018/09/26 15:45:24, Vasily Kuznetsov wrote: > Nit: I think we don't really need to include the information that is already > obvious from the method name. Usually the best docstring for a method is > something like """<verb> [<noun phrase>] ...""" and here it could be just > """Send request to the API and return the response.""" Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:276: 'matchType': self._MATCH_TYPE['update'] if overwrite else On 2018/09/26 15:45:24, Vasily Kuznetsov wrote: > Maybe it would make more sense to have True and False as the keys of this dict > so that you could just index it with `overwrite`. Done. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:399: def get_token(username, password, user_id): On 2018/09/26 15:45:25, Vasily Kuznetsov wrote: > Maybe this should also be a method of XTMCloudAPI class? We have already discussed this in person. As I mentioned then, I feel like `get_token()` doesn't really fit into all the things from `XTMCloudAPI`, as it generates the token, rather than using it. And all the methods of that class make use of the token to make requests. https://codereview.adblockplus.org/29886648/diff/29891625/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/sources.py#newc... cms/sources.py:278: config.write(open(self.get_path('settings.ini'), 'w')) On 2018/09/26 10:43:17, Vasily Kuznetsov wrote: > You need to close the file here. I don't think config.write() will close it. A > nice way to do it would be: > > with open(...) as conffile: > config.write(conffile) Done. https://codereview.adblockplus.org/29886648/diff/29891625/docs/usage/README.md File docs/usage/README.md (right): https://codereview.adblockplus.org/29886648/diff/29891625/docs/usage/README.m... docs/usage/README.md:1: #XTM Integration On 2018/09/26 15:45:25, Vasily Kuznetsov wrote: > Not having space between # and XTM doesn't work with Grip > (https://github.com/joeyespo/grip), which I think uses github renderer, so it > might be better to put it there just to be on the safe side. Done. https://codereview.adblockplus.org/29886648/diff/29891625/docs/usage/README.m... docs/usage/README.md:1: #XTM Integration On 2018/09/26 15:45:25, Vasily Kuznetsov wrote: > Not having space between # and XTM doesn't work with Grip > (https://github.com/joeyespo/grip), which I think uses github renderer, so it > might be better to put it there just to be on the safe side. Done. https://codereview.adblockplus.org/29886648/diff/29891625/docs/usage/README.m... docs/usage/README.md:5: ```commandline On 2018/09/26 15:45:25, Vasily Kuznetsov wrote: > You can also introduce code blocks like this: > > Some text text text: > > code, indented by 4 spaces > > some more code > > This reads better when you're looking at plaintext. Is there an advantage to > backquotes and commandline type? Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/conftest.py File tests/conftest.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/conftest.py#n... tests/conftest.py:12: from tests.xtm_mock_api import get_configured_app On 2018/09/26 15:45:25, Vasily Kuznetsov wrote: > Perhaps it would be better to import this as get_xtm_app or maybe just from test > import xtm_mock_api and then use it with a dot. On the other hand, if you move > this code elsewhere (see comment below), maybe there it will be all about XTM > stuff and this import would be fine. Moved it to another file. Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/conftest.py#n... tests/conftest.py:30: @pytest.fixture(scope='session') On 2018/09/26 15:45:25, Vasily Kuznetsov wrote: > It seems that all this new stuff is just for testing XTM integration so maybe it > would fit better into the file with that test suite. Conftest is supposed to be > more for general stuff that gets used all over the place. Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_site/set... File tests/test_site/settings.ini (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_site/set... tests/test_site/settings.ini:17: [XTM] Actually, yes, I need it here to run the tests. Because some tests would fail, due to this section not being present here. Also, it could work well as an "example" of how settings.ini should be configured in the actual websites. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_api.py File tests/test_xtm_api.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_api.... tests/test_xtm_api.py:15: """Tests for the XTM API integration.""" On 2018/09/26 15:45:26, Vasily Kuznetsov wrote: > Nit: empty lines Also switched the order of the imports :D Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_api.... tests/test_xtm_api.py:58: """Test if the creation of files behaves appropriately.""" On 2018/09/26 15:45:26, Vasily Kuznetsov wrote: > Nit: this could be just "Test creation of files" -- seems like we haven't lost > any information? If you agree, the other doctstrings could be also shortified in > the same way. Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... File tests/test_xtm_translate.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translate.py:15: from __future__ import unicode_literals On 2018/09/26 15:45:26, Vasily Kuznetsov wrote: > Nit: you know Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translate.py:17: import pytest On 2018/09/26 15:45:26, Vasily Kuznetsov wrote: > Nit: pytest should be after all of that stdlib stuff. Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... File tests/test_xtm_translations_utils.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translations_utils.py:15: import pytest On 2018/09/26 15:45:27, Vasily Kuznetsov wrote: > Nit: pytest should be after stdlib. Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translations_utils.py:32: """""" On 2018/09/26 15:45:27, Vasily Kuznetsov wrote: > Nit: probably no sense in having an empty docstring. But maybe here some > explanation would help. Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translations_utils.py:110: exp_out = os.path.join(str(temp_site), *['foo', 'bar', 'faz', 'test.json']) On 2018/09/26 15:45:26, Vasily Kuznetsov wrote: > Isn't this kind of the same as just removing the star and the square brackets? Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_tran... tests/test_xtm_translations_utils.py:189: api.add_target_languages(_PROJECT_ID, ['ro_RO']) On 2018/09/26 15:45:27, Vasily Kuznetsov wrote: > ;) haha :D https://codereview.adblockplus.org/29886648/diff/29891625/tests/utils.py File tests/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/utils.py#newc... tests/utils.py:23: import pytest On 2018/09/26 15:45:27, Vasily Kuznetsov wrote: > Nit: there should be an empty line before pytest, which is not stdlib. Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/xtm_mock_api.py File tests/xtm_mock_api.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/xtm_mock_api.... tests/xtm_mock_api.py:60: auth = request.headers.get('Authorization') On 2018/09/26 15:45:27, Vasily Kuznetsov wrote: > Can this just be `request.headers['Authorization']`? Done. https://codereview.adblockplus.org/29886648/diff/29891625/tests/xtm_mock_api.... tests/xtm_mock_api.py:165: }for tl in xtm_mock.config['target_languages']] On 2018/09/26 15:45:27, Vasily Kuznetsov wrote: > Nit: space before "for"? Done. https://codereview.adblockplus.org/29886648/diff/29891625/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29886648/diff/29891625/tox.ini#newcode21 tox.ini:21: httplib2 On 2018/09/26 15:45:27, Vasily Kuznetsov wrote: > Do we need httplib2 in addition to requests? Sorry if we've discussed it and I > forgot :/ Nope, we don't. Put that in at the very start, forgot to remove it :D Done.
Hi Tudor, There are just a couple of small kinks left (see below) but I think we can already move everything to outside of bin folder: some of the files that still need corrections will not even be moved and that one docstring I can check even though it's moved. Cheers, Vasily https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/__main__.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/__main__.py:1: from cms.bin.xtm_translations.translate_xtm_cloud import main On 2018/10/04 06:48:01, Tudor Avram wrote: > On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > > Most of the code of xtm_translations doesn't need to live in cms/bin (which is > a > > directory for scripts). Perhaps a better approach would be to move everything > > but this file into cms/xtm_translations (maybe even better > cms/translations/xtm, > > just in case we want to have multiple backends at some point) and this file > > could become cms/bin/xtm_translations.py or have any other name that is > > appropriate for the CLI frontend. > > We discussed this in person and, as far as I remember, we decided to do this in > a separate ticket? I think we've decided to do this at the end of this review, after everything else is ironed out. Separate ticket would also work, but I prefer to do it in this review to make the history cleaner. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:388: def get_locales(path, default): On 2018/10/04 06:48:06, Tudor Avram wrote: > On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > > Why don't we use source.list_locales() for this? We should have the source > > around where this is called, right? > > As discussed in person, I need to see all the locales (even the ones where the > directories are empty and which source.list_locales() ignores) Acknowledged. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:414: def write_to_file(data, file_path): On 2018/10/04 06:48:05, Tudor Avram wrote: > On 2018/09/26 10:43:16, Vasily Kuznetsov wrote: > > I wonder if it makes sense to use a similar function from cms.utils for this. > > What do you think? > As discussed in person, there's no such function in `cms.utils`. When doing > refactoring, we might want to put this there and re-use it from > `generate_static_pages.py`, but that would have to be a separate ticket. Acknowledged. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/xtm_api.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:25: _BASE_URL = 'https://wstest2.xtm-intl.com/rest-api/' On 2018/10/04 06:48:09, Tudor Avram wrote: > On 2018/09/26 15:45:24, Vasily Kuznetsov wrote: > > Shouldn't this URL be configurable somehow? > > Going to discuss the level of configurability with Winsley later this week. Acknowledged. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/xtm_api.py:399: def get_token(username, password, user_id): On 2018/10/04 06:48:08, Tudor Avram wrote: > On 2018/09/26 15:45:25, Vasily Kuznetsov wrote: > > Maybe this should also be a method of XTMCloudAPI class? > > We have already discussed this in person. As I mentioned then, I feel like > `get_token()` doesn't really fit into all the things from `XTMCloudAPI`, as it > generates the token, rather than using it. And all the methods of that class > make use of the token to make requests. Allright, makes sense. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_site/set... File tests/test_site/settings.ini (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_site/set... tests/test_site/settings.ini:17: [XTM] On 2018/10/04 06:48:12, Tudor Avram wrote: > Actually, yes, I need it here to run the tests. Because some tests would fail, > due to this section not being present here. > > Also, it could work well as an "example" of how settings.ini should be > configured in the actual websites. > Acknowledged. https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_api.py File tests/test_xtm_api.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/tests/test_xtm_api.... tests/test_xtm_api.py:58: """Test if the creation of files behaves appropriately.""" On 2018/10/04 06:48:13, Tudor Avram wrote: > On 2018/09/26 15:45:26, Vasily Kuznetsov wrote: > > Nit: this could be just "Test creation of files" -- seems like we haven't lost > > any information? If you agree, the other doctstrings could be also shortified > in > > the same way. > > Done. Nice, but you still left "behaves appropriately" in this one :/ The other ones are now perfect btw. https://codereview.adblockplus.org/29886648/diff/29899558/cms/bin/xtm_transla... File cms/bin/xtm_translations/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29899558/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:268: page_strings: dir This function is now great, except for the docstring, which needs to be updated, and the argument naming. Perhaps the argument can be called `local_names` and just be declared to be an iterable (then you can still pass a dict in from the outside). https://codereview.adblockplus.org/29886648/diff/29899558/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29886648/diff/29899558/cms/sources.py#newc... cms/sources.py:392: print(source.read_file('settings.ini')[0]) Oops. https://codereview.adblockplus.org/29886648/diff/29899558/docs/usage/xml-sync.md File docs/usage/xml-sync.md (right): https://codereview.adblockplus.org/29886648/diff/29899558/docs/usage/xml-sync... docs/usage/xml-sync.md:11: ### 1. XTM Login In other parts of CMS documentation we use second level headers (##) for subsection and we leave an empty line below them. I think it would be good to do the same here to be consistent.
Hi Vasily, Here is my ammended code. I addressed all of your comments and added a new section in the documentation, including the external dependencies required to make this work. On top of that, I removed all the TODOs and questions from the comments and created the following two tickets in GitLab: 1. https://gitlab.com/eyeo/websites/cms/issues/6 2. https://gitlab.com/eyeo/websites/cms/issues/7 Thanks, Tudor. https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... File cms/bin/xtm_translations/__main__.py (right): https://codereview.adblockplus.org/29886648/diff/29891625/cms/bin/xtm_transla... cms/bin/xtm_translations/__main__.py:1: from cms.bin.xtm_translations.translate_xtm_cloud import main On 2018/10/05 10:56:24, Vasily Kuznetsov wrote: > On 2018/10/04 06:48:01, Tudor Avram wrote: > > On 2018/09/26 10:43:15, Vasily Kuznetsov wrote: > > > Most of the code of xtm_translations doesn't need to live in cms/bin (which > is > > a > > > directory for scripts). Perhaps a better approach would be to move > everything > > > but this file into cms/xtm_translations (maybe even better > > cms/translations/xtm, > > > just in case we want to have multiple backends at some point) and this file > > > could become cms/bin/xtm_translations.py or have any other name that is > > > appropriate for the CLI frontend. > > > > We discussed this in person and, as far as I remember, we decided to do this > in > > a separate ticket? > > I think we've decided to do this at the end of this review, after everything > else is ironed out. Separate ticket would also work, but I prefer to do it in > this review to make the history cleaner. Done. https://codereview.adblockplus.org/29886648/diff/29899558/cms/bin/xtm_transla... File cms/bin/xtm_translations/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29899558/cms/bin/xtm_transla... cms/bin/xtm_translations/utils.py:268: page_strings: dir On 2018/10/05 10:56:26, Vasily Kuznetsov wrote: > This function is now great, except for the docstring, which needs to be updated, > and the argument naming. Perhaps the argument can be called `local_names` and > just be declared to be an iterable (then you can still pass a dict in from the > outside). Done. https://codereview.adblockplus.org/29886648/diff/29899558/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29886648/diff/29899558/cms/sources.py#newc... cms/sources.py:392: print(source.read_file('settings.ini')[0]) On 2018/10/05 10:56:26, Vasily Kuznetsov wrote: > Oops. haha, that was from Thursday night :)) https://codereview.adblockplus.org/29886648/diff/29899558/docs/usage/xml-sync.md File docs/usage/xml-sync.md (right): https://codereview.adblockplus.org/29886648/diff/29899558/docs/usage/xml-sync... docs/usage/xml-sync.md:11: ### 1. XTM Login On 2018/10/05 10:56:26, Vasily Kuznetsov wrote: > In other parts of CMS documentation we use second level headers (##) for > subsection and we leave an empty line below them. I think it would be good to do > the same here to be consistent. Done.
Hi Tudor, Looks good, just a couple more formalities :) Cheers, Vasily https://codereview.adblockplus.org/29886648/diff/29902566/cms/bin/xtm_transla... File cms/bin/xtm_translations.py (right): https://codereview.adblockplus.org/29886648/diff/29902566/cms/bin/xtm_transla... cms/bin/xtm_translations.py:1: from cms.translations.xtm.cli import main Nit: all Python files that have any content need the license comment at the top https://codereview.adblockplus.org/29886648/diff/29902566/cms/translations/xt... File cms/translations/xtm/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29902566/cms/translations/xt... cms/translations/xtm/utils.py:15: import collections Nit: empty line after the top comment -- I missed it somehow before https://codereview.adblockplus.org/29886648/diff/29902566/cms/translations/xt... cms/translations/xtm/utils.py:261: The local file names(without any extension). Nit: space before the parenthesis
Hi Vasily, I addressed your comments from the previous patch set. There are a couple of extra things that showed up in `sources.py`. That's just from rebasing it with `origin master`. Thanks, Tudor. https://codereview.adblockplus.org/29886648/diff/29902566/cms/bin/xtm_transla... File cms/bin/xtm_translations.py (right): https://codereview.adblockplus.org/29886648/diff/29902566/cms/bin/xtm_transla... cms/bin/xtm_translations.py:1: from cms.translations.xtm.cli import main On 2018/10/05 14:51:24, Vasily Kuznetsov wrote: > Nit: all Python files that have any content need the license comment at the top Done. https://codereview.adblockplus.org/29886648/diff/29902566/cms/translations/xt... File cms/translations/xtm/utils.py (right): https://codereview.adblockplus.org/29886648/diff/29902566/cms/translations/xt... cms/translations/xtm/utils.py:15: import collections On 2018/10/05 14:51:25, Vasily Kuznetsov wrote: > Nit: empty line after the top comment -- I missed it somehow before Done. https://codereview.adblockplus.org/29886648/diff/29902566/cms/translations/xt... cms/translations/xtm/utils.py:261: The local file names(without any extension). On 2018/10/05 14:51:24, Vasily Kuznetsov wrote: > Nit: space before the parenthesis Done. https://codereview.adblockplus.org/29886648/diff/29902698/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29886648/diff/29902698/cms/sources.py#newc... cms/sources.py:1: # This file is part of the Adblock Plus web scripts, The diffs in this file come from rebasing everything with `origin master`.
LGTM
Message was sent while issue was closed.
That's all for now. XTM seems to be stuck in "analysis" so I cannot continue trying this out for now but I may have a few more comments once I can continue :/. I'll try again tomorrow :party:. https://codereview.adblockplus.org/29886648/diff/29902698/cms/translations/xt... File cms/translations/xtm/cli.py (right): https://codereview.adblockplus.org/29886648/diff/29902698/cms/translations/xt... cms/translations/xtm/cli.py:46: username = input_fn('Username: ') This should be the client not the user name (<https://www.xtm-cloud.com/rest-api/#tag/Authentication>). Very confusing since I tried to login via my username (wspee) and the login call never fails (even with wrong password etc) :). https://codereview.adblockplus.org/29886648/diff/29902698/cms/translations/xt... File cms/translations/xtm/xtm_api.py (right): https://codereview.adblockplus.org/29886648/diff/29902698/cms/translations/xt... cms/translations/xtm/xtm_api.py:26: _BASE_URL = 'https://wstest2.xtm-intl.com/rest-api/' This is a hardcoded url to a user specific test system. Not sure it needs to be configure/changeable but at least for production it should be <https://eu1.xtm.cloud/rest-api/>? https://codereview.adblockplus.org/29886648/diff/29902698/docs/usage/xml-sync.md File docs/usage/xml-sync.md (right): https://codereview.adblockplus.org/29886648/diff/29902698/docs/usage/xml-sync... docs/usage/xml-sync.md:41: * *--source-lang* = The language of the source files. If not provided, This parameter is not mandatory https://codereview.adblockplus.org/29886648/diff/29902698/docs/usage/xml-sync... docs/usage/xml-sync.md:43: * *--save-id* = Whether to save the id of the newly created project into This parameter is not mandatory https://codereview.adblockplus.org/29886648/diff/29902698/docs/usage/xml-sync... docs/usage/xml-sync.md:64: python -m cms.bin.xtm_translations [-h] [-v] upload [-h] [--no-overwrite] For a per issue workflow (and also in general) it would be very nice to be able to specify the project id via the command line. https://codereview.adblockplus.org/29886648/diff/29902698/docs/usage/xml-sync... docs/usage/xml-sync.md:64: python -m cms.bin.xtm_translations [-h] [-v] upload [-h] [--no-overwrite] Also it might make sense to document the settings.ini parameter? 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
