python-sc2 icon indicating copy to clipboard operation
python-sc2 copied to clipboard

Simplified map and path behavior and added other basic functionality

Open Azsu opened this issue 2 years ago • 5 comments

Simplify map/path usage in run_game Fix misnomer of map_settings -> map_path Refactored calls to run_game and maps.get Added Units.health_highest, Units.health_lowest Added BotAI.enemies_all and deprecated BotAI.all_enemy_units and misc cleanup Ignoring virtual environment

Azsu avatar Jun 22 '22 10:06 Azsu

I don't quite understand your automated check failures looking at the code and I don't get them locally:

Run poetry run pylint $(git ls-files '*.py' | grep -E 'sc2/.*')
************* Module sc2.main
sc2/main.py:775:0: C0325: Unnecessary parens after 'if' keyword (superfluous-parens)
************* Module sc2.units
sc2/units.py:217:0: C0325: Unnecessary parens after 'return' keyword (superfluous-parens)
------------------------------------
Your code has been rated at [10](https://github.com/BurnySc2/python-sc2/runs/7002380512?check_suite_focus=true#step:9:11).00/10

vs

bash-3.2$ poetry run pylint $(git ls-files '*.py' | grep -E 'sc2/.*')

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

Azsu avatar Jun 22 '22 10:06 Azsu

Regarding the FileNotFoundError: Map AcropolisLE not found

This is only happening on Unix systems minus OSX, I'm not sure why this is happening as this should mostly just be a refactor. If the map does exist in that image can you point me to the Dockerfiles so I can repo? Do the Unix systems require an SCPATH to be defined?

Azsu avatar Jun 22 '22 10:06 Azsu

I can't easily identify your changes because the changes + autoformatting were done in one commit :frowning:

BurnySc2 avatar Jun 22 '22 15:06 BurnySc2

The primary change that's impacting these checks is the refactoring of map.py and path.py to file_map.py and file_path.py, to disambiguate the map files from the in-game map.

I also refactored the get function into the class and then renamed map_settings to map_path as it was a misnomer and made the run_game map parameter a Union[MapPath, str] to simplify user behavior.

It all seems to work pretty well on my OSX including the addition of the alternate_install_path parameter that I added to run_game.

Azsu avatar Jun 23 '22 19:06 Azsu

I can't easily identify your changes because the changes + autoformatting were done in one commit 😦

Would it help if I retracted this, ran black on the structure, then just submitted a delta with Blacks's formatting suggestions? I feel like it does a significantly better job of improving the readability of the code than the current formatter.

Azsu avatar Jun 24 '22 03:06 Azsu

I'm going to decline this PR for now because I am unable to review it with most of the changes being formatting changes by Black formatter, even though I already decided on the yapf formatter. Also this PR seems to introduce bot-breaking changes. If that is done for a good reason (e.g. mostly performance or ease of use), I would normally allow it. It would be better to have the functional changes in a separate reviewable commit, and the styling (formatting) changes in a different commit (or even a discussion about it beforehand would be preferable).

BurnySc2 avatar Jan 09 '23 03:01 BurnySc2