Page MenuHomeWildfire Games

tools/entity/checkref perl to python
ClosedPublic

Authored by Stan on Dec 11 2020, 8:52 PM.

Details

Summary

Updated patch for D2849

Called as a class, tested that did similar output to perl.

Test Plan

See if it is enough.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mammadori requested review of this revision.Dec 11 2020, 8:52 PM
mammadori created this revision.
mammadori updated this revision to Diff 14570.Dec 11 2020, 11:22 PM

"fixed" path for windows

Stan added a comment.Dec 12 2020, 9:06 AM

Some questions, can we do it in parallel? Do you think tqdm would slow it a lot?

source/tools/entity/checkrefs.py
171 ↗(On Diff #14570)

I get this error now:

Traceback (most recent call last):
  File "C:\Dev\trunk\source\tools\entity\checkrefs.py", line 475, in <module>
    CheckRefs().main()
  File "C:\Dev\trunk\source\tools\entity\checkrefs.py", line 51, in main
    self.add_entities()
  File "C:\Dev\trunk\source\tools\entity\checkrefs.py", line 170, in add_entities
    phenotype_tag = entity.find('Identity').find('Phenotype')
AttributeError: 'NoneType' object has no attribute 'find'
source/tools/entity/scriptlib/__init__.py
56 ↗(On Diff #14570)

Apparently we also support mul_round can you add it? (it's mul + rounding)

elif op == 'mul_round ':
    base_tag.text = str(round(op1 * op2))
mammadori updated this revision to Diff 14596.Dec 13 2020, 8:23 AM

Changed path handling, added mul_round

Stan added a comment.Dec 13 2020, 12:11 PM

I get a new error now :/ I guess it's because it tries to open files with native encoding instead of UTF-8

Loading civs...
Traceback (most recent call last):
  File "C:\Dev\trunk\source\tools\entity\checkrefs.py", line 478, in <module>
    check_ref.main()
  File "C:\Dev\trunk\source\tools\entity\checkrefs.py", line 61, in main
    self.add_civs()
  File "C:\Dev\trunk\source\tools\entity\checkrefs.py", line 368, in add_civs
    civ = load(f)
  File "C:\Python39\lib\json\__init__.py", line 293, in load
    return loads(fp.read(),
  File "C:\Python39\lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
UnicodeDecodeError: 'charmap' codec can't decode byte 0x9d in position 3419: character maps to <undefined>

Also if I may, is it possible to have the prints as the script goes, instead of all when it fails? Maybe this is a windows bug?

mammadori updated this revision to Diff 14598.Dec 13 2020, 1:36 PM

Explicit utf8 opening.

In D3213#142105, @Stan wrote:

Also if I may, is it possible to have the prints as the script goes, instead of all when it fails? Maybe this is a windows bug?

I don't know how your Windows console handles stdout, try with some print() and sleep() to reproduce eventually. I just did the minimum intervention, but it probably needs some refactoring and logging improvements. I don't know how often this is used and by whom for what ;-)

Stan added a comment.Dec 13 2020, 3:49 PM

Seems using import logging seems to work nicer on windows. Here is a naive implementation https://pastebin.com/4Rr067PW (you need to replace all the prints by their counter parts)

The utf8 change did the trick.

-t is still broken

Traceback (most recent call last):
  File "C:\Dev\trunk\source\tools\entity\checkrefs.py", line 489, in <module>
    check_ref.main()
  File "C:\Dev\trunk\source\tools\entity\checkrefs.py", line 82, in main
    proc = run([str((Path(__file__).parent / '../xmlvalidator/validator.py').resolve())])
  File "C:\Python39\lib\subprocess.py", line 501, in run
    with Popen(*popenargs, **kwargs) as process:
  File "C:\Python39\lib\subprocess.py", line 947, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "C:\Python39\lib\subprocess.py", line 1416, in _execute_child
    hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
OSError: [WinError 193] %1 is not a valid Win32 application.
source/tools/entity/checkrefs.py
105 ↗(On Diff #14598)

ffp is unused

149 ↗(On Diff #14598)

Unused.

153 ↗(On Diff #14598)

Linter says i is unused

443 ↗(On Diff #14598)

if dep.replace("/", os.path.sep) in uniq_files: works it seems but as said on IRC it's not the correct approach.

mammadori marked 4 inline comments as done.Dec 14 2020, 5:10 PM
mammadori added inline comments.
source/tools/entity/checkrefs.py
105 ↗(On Diff #14598)

is used to store output return value

149 ↗(On Diff #14598)

yep, linter and pep8 too, same as above, used to store output from sorted, but you cannot simply delete it without refactor a bit

171 ↗(On Diff #14570)

Do still happens?

mammadori updated this revision to Diff 14615.Dec 14 2020, 5:13 PM
mammadori marked 2 inline comments as done.

Included logging, and the bad separator fix

mammadori updated this revision to Diff 14616.Dec 14 2020, 6:22 PM

Fixed invoking with -t in windows (also is python, invoke a module)

Stan added a comment.Dec 14 2020, 6:32 PM

Nice! Will try to test it soon (not tonight as something came up) i think you forgot to remove the entity.pm file :)

In D3213#142287, @Stan wrote:

Nice! Will try to test it soon (not tonight as something came up) i think you forgot to remove the entity.pm file :)

I saw a comment in the original ticket about deleting in another commit, in the first one I delete it. As you wish.

Stan added a comment.EditedDec 17 2020, 5:07 PM

Sorry it took me so long to reply.

Well, for Entity PM if you now have 100% of the functionnality it is now useless.

It seems validate.py is missing some support for variants. It seems it's also different from validate.pl, which we currently use. Any hope you could merge the functionnality?

Seems rP24215 broke the script

bb retitled this revision from Fixed patch for D2849 to tools/entity/checkref perl to python.Dec 22 2020, 11:31 PM
Stan added a comment.Feb 11 2021, 1:40 AM

Hey there any news?

Stan commandeered this revision.Feb 10 2022, 12:15 AM
Stan edited reviewers, added: mammadori; removed: Stan.
This revision was not accepted when it landed; it landed in state Needs Review.Feb 12 2022, 4:43 PM
This revision was automatically updated to reflect the committed changes.