Starting editor from command line is broken

[squiddy]

$ python run_uh.py --edit-map development
Error: 'development' is not a valid Unknown Horizons map or savegame file.

A quick check suggests that the fix for #2507 (PR #2651) broke this functionality. @AndyMender

[squiddy]

I already added a test (currently failing) in e87bbde

[AndyMender]

@squiddy, thank you for letting me know. I will have a look at this asap. Not sure why the test would be failing, though. Maybe I should expand the "map-satisfying" conditions?

EDIT: At least the error message is pretty clear :P.

[squiddy]

Not sure why the test would be failing, though.

Not sure I understand. The test is failing because the functionality is broken.

[AndyMender]

So it seems that when one tries to start a scenario from the command-line, the set of scenarios (object savegames) is a dict of the format defined in the _find_matching_map docstring. However, when the editor is launched, savegames is actually a zipped tuple (but not a zip object) of the format: ( (path1, path2, path3,...) , [map1, map2, map3,...] ), where pathN is the relative path of mapN.

This requires an entirely different approach to the look-up algorithm.

[squiddy]

IMO we should write more tests for the command line interface, similar to e87bbde. Every usage of _find_matching_map should be covered by a simple test. That would be:

• --start-scenario X (where X is a path to a file)
• --start-dev-map
• --start-map X
• --string-previewer

Once we got those covered we should have more confidence in refactoring things. Maybe we don't want a single function that can do three things at once: load maps, load savegames, load scenarios.

[squiddy]

Without tests, the next change may break new things. We should strive to increase test coverage everytime we change things.

[AndyMender]

The issue closed by itself, apologies. For now the code covers the following:

python run_uh.py --start-scenario=tutorial
python run_uh.py --edit-map development
python run_uh.py --edit-map content/maps/development.sqlite
python run_uh.py --start-dev-map
python run_uh.py --start-map development

I tried to make it compatible with both the --start-scenario option and the --edit-map option, but I feel it's far from perfect.

PS I agree on the tests, of course!

EDIT: A good idea might also be to change the functions triggered by the individual command-line options to return savegames in a consistent format.

EDIT2: development/stringpreviewwidget.py is broken as it escaped the 2to3 "purge" somehow. Will fix it soonish.

[squiddy]

Added more tests in 4860a2d

Starting a scenario with a specific path seems to be broken.

[AndyMender]

That is correct since the _find_matching_map function does not yet handle such a scenario. I will add it, therefore :).

[AndyMender]

I added handling for the unittest that was failing:
python run_uh.py --start-scenario content/scenarios/tutorial_uk.yaml
and also for absolute paths to the savegame/map .sqlite and scenario .yaml files.

[squiddy]

Once we have tests for the load-game functionality, I'd like to revisit the current implementation. I feel like it is more complex than before. We did however fix the scenario bug.

Having one function for all these things isn't a good idea IMO

[squiddy]

I'm probably going to write these tests this evening. I don't like the workaround I have to do, but that's a minor issue compared with having no test at all.

[AndyMender]

The current _find_matching_map function is more complex than before, because it tries to address both the scenario-containing dict and the map tuple very explicitly. I don't think that's good at all, because the structure of the savegames object is vastly different for both (not to mention that it's not "savegames" anymore in the case of the map editor). To that end, I will probably split the _find_matching_map function into 2-3 functions, depending on the context they're used in.

I think we also need a separate ticket to track our unittest coverage, right? @squiddy, could you open one, since you know best which tests still need to be implemented? :)

[squiddy]

We still need more tests, this whole refactoring basically broke all gui tests that relied on loading specific scenarios / maps that are not in the content/ directory.

$ python run_uh.py --start-scenario tests/gui/scenarios/win.yaml
Traceback (most recent call last):
  File "run_uh.py", line 372, in <module>
    main()
  File "run_uh.py", line 182, in main
    ret = horizons.main.start(options)
  File "/home/reiner/projekte/unknown-horizons/horizons/main.py", line 199, in start
    startup_worked = _start_map(command_line_arguments.start_scenario, 0, True, force_player_id=command_line_arguments.force_player_id)
  File "/home/reiner/projekte/unknown-horizons/horizons/main.py", line 420, in _start_map
    map_file = _find_matching_map(map_name, savegames)
  File "/home/reiner/projekte/unknown-horizons/horizons/main.py", line 477, in _find_matching_map
    name_or_path, game_language = os.path.splitext(os.path.basename(name_or_path))[0].split("_")
ValueError: not enough values to unpack (expected 2, got 1)

[AndyMender]

Unfortunately, I had to implement some gimmicks for the path string, because the alternative was extracting the path from the savegames dict, which would be equally hackish. I think we need some sort of a consensus on which scenario loading approaches do we support (like the custom path method you specified).

I'll have a look at the code again and see how can I rebuild it or implement custom scenario/map paths. Also, I think it's a good idea to separate functions loading scenarios from those loading maps for the editor.

[AndyMender]

@squiddy, I need to separate this code to be handled by two different functions:
https://github.com/unknown-horizons/unknown-horizons/edit/master/horizons/main.py#L416

[AndyMender]

I split the _find_matching_map function into _find_scenario and _find_map functions. Both are quite verbose, but they cover multiple ways of loading the .sqlite and .yaml files. To that end, _find_scenario is used only by _start_map if a game language locale is specified.

EDIT: @squiddy, could you have a look whether the tests are running fine now?

[squiddy]

This particular issue is fixed.