tModLoader tModLoader.CodeAssist overhaul

What is the new feature?

Reworked analyzers for tML.

Renamed tModLoader.CodeAssist to tModCodeAssist to fit with the tML branding
Moved tModCodeAssist to the tML repository and added it as an for mods analyzer in tModLoader.targets
Ported the old analyzer rules
Added new rules

Why should this be part of tModLoader?

Having tModCodeAssist in the tModLoader repository is much better than having it separately and publishing it as a nuget package because we don't have to consider supporting 1.3, 1.4.3, and 1.4.4 at the same time in one codebase. New analyzer rules will also help new modders port vanilla code and can help them simplify their own code.

Are there alternative designs?

Leaving the name as tModLoader.CodeAssist (bad)
Leaving the analyzer in a separate repository and making tML a submodule of it (bad)
Leaving the analyzer as a nuget package (bad)

Sample usage for the new feature

ExampleMod updates

N/A

TODO

[ ] Magic Number rules
[ ] Port rand simplify rule
[ ] Implement Array Range rule
E.g. NPC.ai[4] will error now.
[ ] Implement Item.value = Item.buyPrice(...) rule
E.g. Item.value = 560 will be suggested to change to Item.value = Item.buyPrice(silver: 5, copper: 60)
[ ] Report usage of System.Random (and replace with Main.rand, that's what used in 99.9% cases anyway)
[ ] Implement analyzer that will detect bad usage on ModContent.GetInstance<T>()
E.g. GetInstance<MyItem>().Foo = 5; will warn modder
[ ] Item.maxStack = 9999 -> Item.maxStack = Item.CommonMaxStack rule
[ ] Add more potential analyzers!

Sep 25 '23 02:09 BasicallyIAmFox

Points of concern:

I believe some mods are evil and actually extend the ai arrays to allow for additional values,
there are valid reasons for mods to want to "reset" the research count to 1 (i.e. tweaking another mod).

I understand that analyzers can be configured, and I'm perfectly fine with the former point being a configurable warning (certainly should not be an error though).

The latter should always be a hint (which is how it appears to be based on the screenshot), but I want to establish this as a standard. Changing/resetting research counts is very reasonable mod behavior unlike extending the length of the ai array, so I'd like to make sure we leave these suggestions as easily-ignorable hints.

Just my two cents.

Sep 25 '23 16:09 steviegt6

Took a glance at the content of the PR and:

might it be better to shove annotations and IdDictionary instances in a TML partial class for IDs,
and would it make more sense to do tML annotations through an XML file or something instead of manually patching each field and class?

For 2), it just reduces patch quantity and sizes, but may also make it easier to keep tML up-to-date. Introducing this infrastructure can make it easier to annotate dependent mods or other external dependencies if they aren't annotated.

Sep 25 '23 16:09 steviegt6

Took a glance at the content of the PR and:
1. might it be better to shove annotations and IdDictionary instances in a TML partial class for IDs,

2. and would it make more sense to do tML annotations through an XML file or something instead of manually patching each field and class?
For 2), it just reduces patch quantity and sizes, but may also make it easier to keep tML up-to-date. Introducing this infrastructure can make it easier to annotate dependent mods or other external dependencies if they aren't annotated.

will do but later. Not major priority atm. As for 2), it sounds interesting, but I have no clue how to do it, unfortunately.

Sep 25 '23 16:09 BasicallyIAmFox

Points of concern:
* I believe some mods are evil and actually extend the `ai` arrays to allow for additional values,

* there are valid reasons for mods to want to "reset" the research count to 1 (i.e. tweaking another mod).
I understand that analyzers can be configured, and I'm perfectly fine with the former point being a configurable warning (certainly should not be an error though).

The latter should always be a hint (which is how it appears to be based on the screenshot), but I want to establish this as a standard. Changing/resetting research counts is very reasonable mod behavior unlike extending the length of the ai array, so I'd like to make sure we leave these suggestions as easily-ignorable hints.

Just my two cents.

I haven't seen any mod that modifies research count. Could you point me any mods doing that?

Sep 25 '23 16:09 BasicallyIAmFox

how do i reopen this lmao

Mar 08 '24 15:03 BasicallyIAmFox

Recreate the PR and stop rewriting history so much. We squash PRs, so you don't need an overly clean history in PRs.

Mar 10 '24 16:03 Mirsario

Recreate the PR and stop rewriting history so much. We squash PRs, so you don't need an overly clean history in PRs.

wtf you mean "recreate the pr"? i wanted to rewrite the pr from scratch either way

Mar 10 '24 21:03 BasicallyIAmFox