search

open-contracting-archive / extensions-data-collector

Superseded by open-contracting/extension_registry.py
https://github.com/open-contracting/extension_registry.py
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Add translation into extension-collecting flow #23

Closed rhiaro closed 6 years ago

odscjames commented 6 years ago

My first comment is that something has gone wrong with someone's flake8. https://github.com/open-contracting/extensions-data-collector/blob/master/.flake8#L3 clearly sets max line length to 119, but if you look at many of the changes to ocdsextensionsdatacollector/runner.py many of the changes are to get the line length under 80 chars (the default). Can this be resolved? We should generally make sure everyone has good tools anyway, but also, if we could then undo the erroneous changes to ocdsextensionsdatacollector/runner.py the diff for that file would be fair easier to read. Thanks :-)

odscjames commented 6 years ago

So, are we happy with the idea that in this repo the translated string files are NOT going to be checked into the repo? That seems to make sense, but as with https://github.com/open-contracting/extension-explorer/pull/34#pullrequestreview-160745749 I just wanted to check this is ok rather than assuming.

However, that does mean it's only possible to run this IF you have a write Transifex key, as the readme states. Is it possible to get round this? As that really limits who can actually run it. Is it possible to run it without such a key, and in that mode, it won't push (cos' it can't) but it will pull? (I don't know if that is possible with Transifex?)

jpmckinney commented 6 years ago

I think the changes to ocdsextensionsdatacollector/runner.py are fine, as they make the code more DRY, instead of constantly having very deep dictionary traversals, which is more prone to error (among other reasons in favor of not having repetitive code). I don't think the changes are erroneous.

Update: Sorry, I was only looking at the last commit that mentioned pep8 – I see some earlier changes now that break lines that don't need to be broken. It looks like pep8 might have been run instead of flake8, and pep8 won't look at .flake8 AFAIK.

kindly commented 6 years ago

@odscjames yes that is odd I did not notice the pep8. Some of the changes were on purpose though as @jpmckinney said.

Also the long term plan is to have another repo with a backup of the po files from transifex. Transifex will still be the single source of truth but we will have a copy of the files. When we have done that it will fairly easy to let the collector use those files as an alternative to needing transifex access. However, I do not think that is needed for this pull request.

jpmckinney commented 6 years ago

Merging as-is. I'll fix unnecessary line breaks separately.