Refactor blazemod [RDY]

[mugling]

Cleanup of #17417 using #17431

[mugling]

Done. No assessment as to balance just refactoring of the JSON using the lint tool

[codemime]

The PR is damn huge.

[mugling]

The PR is damn huge.

Yes, no way around that - reformatting only adds ~30% versus the parent PR (#17417)

[Coolthulhu]

Is it tested or only reformatted?

[Coolthulhu]

This line is horrible. Can linter detect and remove those too?

[mugling]

This line is horrible. Can linter detect and remove those too?

I could extend it to provide a NOEMPTY output flag

[Coolthulhu]

Aren't "count": 1 and "stack_size": 1 redundant?

[mugling]

Yes, but that is out of scope of the linter - it's valid JSON, a valid field and sensibly formatted.

Strict mode (#17722 etc) could detect this but cannot be enabled by default for mods due to the false positives that would be reported due to the variation in load order. One option would be a command-line switch that selectively tested a mod in strict mode and output a non-compliance log?

[Coolthulhu]

This entire line of items is a giant block of horrible copy+paste. Would benefit from some "similar json detector" system that would replace sets of definitions with copy-from entry.
Extending linter to do that would probably be too much work (it's too specialized), but I could write a custom script just to do that for this PR.

[mugling]

Is it tested or only reformatted?

It is only reformatted. I haven't addressed balance or anything like that.

[Coolthulhu]

I haven't addressed balance or anything like that.

Balance can be off (most likely is), but someone will have to bite the bullet and go through all of this crap to check if it actually works or spews debugmsgs on every attempt.

Or we can just say "fuck it" and merge it with the assumption that mod content can be full retard. But then we'll have to maintain it.

Yes, but that is out of scope of the linter - it's valid JSON, a valid field and sensibly formatted.

Could you run a separate script for that? Some simple jq equivalent of:

if( entry["type"] == "ammo" && entry["count"] == 1 ) entry.erase( "count" );

and ditto for "stack_size".

The PR is unreviewably huge and ugly. Some quick and dirty solutions may be necessary to trim it to manageable size.

[mugling]

Yes, some jq-fu could be in order

Or we can just say "fuck it" and merge it with the assumption that mod content can be full retard.

It's better by far than the status quo and to be honest I can only justify further effort in this PR in that context.

But then we'll have to maintain it.

This is effectively a one-time deal. We should do everything possible to avoid getting to this situation again. Maintenance should be more practical given the lack of JSON errors and the author becomes responsible for any fixes internal to the mod.

Summarily if I run this through jq and possibly sed and get no load errors are you agreeable to a merge?

[Coolthulhu]

Summarily if I run this through jq and possibly sed and get no load errors are you agreeable to a merge?

I'd still have objections. This needs automated testing.
I can write a unit test specifically for it.
Since we're essentially re-mainlining a mod, it should at the very least work. Doesn't need to work well, but should work.

I see a unit test like this:

• Load blazemod
• For every single turret in the mod:
• Spawn single-frame vehicle
• Install full storage battery and the turret on the vehicle
• Find a holder for the default ammo of this turret
• If it exists, install a holder and fill it to the brim with default ammo
• If it doesn't, try to fill the turret instead
• Fire the turret at random tile within its range
• Wait a turn
• If no debugmsgs happen, continue, otherwise print that this turret is wonky and mark the test as failure

I assume many turrets will fail it, so whatever commands you're using to clean this up you should save in a script. We'll have to work with blaze to make the mod at least work before it's really mergeable, so we'll most likely have to clean it up more than once.

[BorkBorkGoesTheCode]

Do you need help?

[Zireael07]

When is this coming?

[mugling]

Waiting on #17746

[BorkBorkGoesTheCode]

Are you still interested in merging this?

[mugling]

Needs a rebase then some fixes to pass the unit tests but we should be otherwise ready. @Coolthulhu anything else?

[Coolthulhu]

Once it passes the firing test and is mergeable, it should be better than the broken version we have in master.

[mugling]

This is very difficult to rebase

[mugling]

0227c16 fixes a bug whereby specifying terrain:symbol or terrain:color as an array with one element results in a cryptic error message. The new version supports loading a string, an array with a single element or an array of four elements. Anything else yields a descriptive error message.

Almost ready:

• ./cataclysm-tiles --check-mods blazemod
• gmake json-lint
• ./tests/cata_test --mods=blazemod [gun]

[mugling]

Passes all automated tests and considerably easier to work on than before. No doubt the mod contains other issues but I'm going to merge this as per the above discussion.