IATI / refresher

A Python application which has the responsibility of tracking IATI data from around the Web and refreshing the core IATI software's data stores
GNU Affero General Public License v3.0
2 stars 0 forks source link

2023 10 18 #290

Closed odscjames closed 11 months ago

simon-20 commented 11 months ago

Looks okay. A couple of questions:

odscjames commented 11 months ago

In flatten.py, line 30, is there a reason for raising a general exception and not something more informative?

We don't need more detail than FlattenerException at the moment, anything there will just be logged as an error for us to look at later.

On line 186, the time.sleep(360) used to re-try potentially "soft" 404s/500s seems like it has the potential to introduce a lot of variation in processing times; e.g., if the flattener encountered a few of these in short succession.

That's all being removed soon anyway, and this new Python class called directly instead.

Probably irrelevant to the test itself, but is there a reason why the first entry in some of the '*_xml_lang' arrays in src\t ests/fixtu/res_flatten_flatterer/historic_date_format.expected.json (and some of other test files) are empty and not "en"?

Can you be clear exactly where you mean, maybe use the comment on line feature on github? All the _xml_lang examples I can see have values.

(Make sure you are looking at changes on whole PR, some early commits have empty values here but then they are removed by the "Flattener tests: Remove expected NaN's, 1970 dates and empty strings from arrays" commit in the middle.)

simon-20 commented 11 months ago

Probably irrelevant to the test itself, but is there a reason why the first entry in some of the '*_xml_lang' arrays in src\t ests/fixtu/res_flatten_flatterer/historic_date_format.expected.json (and some of other test files) are empty and not "en"?

Can you be clear exactly where you mean, maybe use the comment on line feature on github? All the _xml_lang examples I can see have values.

(Make sure you are looking at changes on whole PR, some early commits have empty values here but then they are removed by the "Flattener tests: Remove expected NaN's, 1970 dates and empty strings from arrays" commit in the middle.)

Yeah, was just looking at one commit.

Don't know whether you wanted me to approve the changes in addition to Alex, but I've gone ahead and done it.