Fixed linter errors in the Python scripts #175
Conversation
babolivier
left a comment
babolivier
left a comment
There was a problem hiding this comment.
This looks great, thanks a lot! I've noticed a (small) issue with the bash command, and I've got a few other nitpicks I'd like to see addressed before I'm happy to merge this.
tools/convert.py
Outdated
| failed_files[f] = e | ||
|
|
||
| if len(failed_files) > 0: | ||
| if failed_files: |
There was a problem hiding this comment.
I prefer explicitly stating the condition we're checking here (i.e. to keep this line as it is, even though it does essentially the same thing), I find it makes the code a bit easier to read.
There was a problem hiding this comment.
I will revert this change. See FURB115 for an explanation of the lint rule.
There was a problem hiding this comment.
I think at this point it comes down to personal preference. I have a tendency to favour readability over making code as succinct as it can be, maybe sometimes to a fault - not that it's necessarily a huge deal in this case.
| files=( ispdb/!(*.xml) ) | ||
| if (( ${!files[*]} )); then | ||
| for file in "${files[@]}"; do | ||
| printf '::error file=%s::File name "%s" does not end in .xml – Please rename!\n' "$file" "$file" |
There was a problem hiding this comment.
While we're here, we could replace this printf with echo (and remove the ::error file=%s:: bit since it's a bit redundant).
There was a problem hiding this comment.
It looks like the idea in 2921ddd was to use GitHub Actions annotations, so that it would annotate the problematic file.
There was a problem hiding this comment.
Hm, fair enough, it should probably stay then. I still think we should move away from printf to echo but I won't block on this.
| files=( ispdb/!(*.xml) ) | ||
| if (( ${!files[*]} )); then | ||
| for file in "${files[@]}"; do | ||
| printf '::error file=%s::File name "%s" does not end in .xml – Please rename!\n' "$file" "$file" |
There was a problem hiding this comment.
Hm, fair enough, it should probably stay then. I still think we should move away from printf to echo but I won't block on this.
2 participants
We could remove the dependency on lxml as well, as Python has built in XML parsing in the xml.etree.ElementTree module, which would improve CI performance.