Preliminary formatting changes #518

Steven-Biro · 2021-02-21T01:58:13Z

This formats all the code in the repo and (eventually) will check formatting and checking that it can be built in circleci before allowing stuff to be merged in

FF1Lib/Dialogs.cs

…ent that always results in false in spellcrafter, need to come back to this and see if it is the same end result

…e autoformatter to crash

wildham0 · 2021-02-24T13:02:54Z

FF1Lib/ItemLists.cs

-			return incentivePool.Concat(ItemLists.AllQuestItems).Distinct().ToList();
+			List<Item> incentivePool = new();
+
+			if (flags.IncentivizeMasamune ?? false)


this might be the only thing that's really bothering me, as this change lowers readability imo, but this might be also a sign that these flags should be handled differently

I would have hoped that it would at least make one-liners to make the whole thing a little more compact.

I agree that this makes it less readable, though in most cases cramming as much into one line as possible isnt great imo, so as a general rule I think this is good, but this function likely needs exceptions.

Make a list that maps flags to items and iterate over the list with a simple function.

wildham0 · 2021-02-24T13:05:39Z

this looks good to me
one question: what's the criteria for keeping lists' items on a single line or putting them on each of their line? going over everything it seems a bit random what list get expanded that way or not

mikelattanziobluescape · 2021-02-24T13:51:04Z

this looks good to me
one question: what's the criteria for keeping lists' items on a single line or putting them on each of their line? going over everything it seems a bit random what list get expanded that way or not

If it's anything like prettier it switches to multiline when it doesn't fit within the line length you specify. If we don't explicitly set one somehow it's probably like 120 chars.

In my experience, forced style is worth the price of nobody being 100% happy with what it looks like, as long as IDE integrations can be tweaked to everybody's liking. I've been a format-on-save dude for a few years now, and just not even thinking about pressing enter or spacing things properly while coding, knowing that just saving once makes everything 'perfect' is a cool thing to get used to.

mhn65536 · 2021-02-24T15:12:43Z

FF1Lib/ExtensiveHints/HintSources/FloorHintsHintSource.cs

-			HashSet<Item> MehItems = new HashSet<Item>(ItemLists.RareWeaponTier.Concat(ItemLists.RareArmorTier).Distinct());
-			HashSet<Item> SparkleItems = new HashSet<Item>(ItemLists.AllGoldTreasure);
-
-			var locations = rom.generatedPlacement.Where(p => p is TreasureChest).GroupBy(p => p.MapLocation);


Why is var not allowed here?

because the current rules chooise IDE0008 over IDE0007

https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0007-ide0008

Personally, I do prefer explicit types, but this is all up for discussion

I like var (I mean, it's less work), but since we're manipulating a lot of lists and arrays explicit types are way clearer at first glance and should make it easier for newcomers to figure out what the randomizer is doing

one thing that is nice, is you can program it with var, then run the formatter and it will try to replace it with the correct type (it only failed a couple times for me and that was when I had a config issue resulting in the formatter not understanding what a Blob is)

2 years ago i preferred explicit types as well. But when using linq, things can get out of hands very easily like:

foreach (IEnumerable<IGrouping<KeyValuePair<string, Tuple<string, string>>> item in list)

and become really readable.

Hmm. I like var for pretty much everything except primitive types like int and ushort. Usually when you're doing a complicated LINQ statement, you don't really care what the exact type is because you're just going to iterate over it in a sec. You can always hover to see the type anyway if you need to.

mhn65536 · 2021-02-24T15:14:07Z

FF1Lib/Fun.cs

 					{
-						0x7C649, 0x7C6F9, 0x7C75A, 0x7C75B, 0x7C62D, 0x7C75D,


In my opinion, inflating that list is also not ideal.

I agree, this is likely a rule we need to tweak, or we need to add a few exceptions

Is it possible to disable the style checker for one line with a comment?

mhn65536 · 2021-02-24T15:14:57Z

FF1Lib/Fun.cs

-				Data[0x31F85 + i * 2] = (byte)(xCoord - 0x10);            // X Coord of Character targeting cursor
-				Data[0x31F86 + i * 2] = (byte)(yCoord + 4 + ((i % 4) * 0x1C));  // Y Coord of Character targeting cursor
-				Data[0x31F76 + i * 2] = (byte)(0xA7 + (Math.Min(i, 4) % 4 * 0x10)); // Y Coord of Command Menu cursor (last four are identical)
+				Data[0x31F85 + (i * 2)] = (byte)(xCoord - 0x10);            // X Coord of Character targeting cursor


we don't have to do this, do we?

this does not change the functionality, it is the result of choosing IDE0048 over IDE0047
https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0047-ide0048

basically, the formatter will automatically add parentheses anywhere that might cause confusion, so people later on dont make a mistake when reading the code.

but that operator precedence you usually learn around the age of 10

mhn65536 · 2021-02-24T15:16:23Z

FF1Lib/ItemLists.cs

-			return incentivePool.Concat(ItemLists.AllQuestItems).Distinct().ToList();
+			List<Item> incentivePool = new();
+
+			if (flags.IncentivizeMasamune ?? false)


I would have hoped that it would at least make one-liners to make the whole thing a little more compact.

mhn65536 · 2021-02-24T15:18:16Z

FF1Lib/LegendaryShops.cs

-		Dictionary<string, MagicSpell> Spells;
-
-		List<SpellInfo> SpellInfos;
+		private readonly MT19337 rng;


if you kill my whitespace, im leaving...

Just joking, but in my opinion this goes too far.

The rules are up for tweaking, I'll try to find the rule that enforces no blank lines here and we can discuss it as a group.

Oh, yeah. Single blank lines should always be acceptable. I'm okay with the linter removing multiple blank lines or blank lines before close braces.

mhn65536 · 2021-02-24T15:21:19Z

FF1Lib/LegendaryShops.cs

@@ -91,7 +91,10 @@ public void PlaceShops()

 			ShopData.StoreData();

-			for (int i = 0; i < 8; i++) TileSets[i].StoreData();
+			for (int i = 0; i < 8; i++)


I'd strongly prefer this to be a one-liner. In my opinion the extra lines can make already long code even longer and less readable.

I know a lot of this is personal preference, but at one point you have to place some trust into a programmer to code it in an understandable and readable way.

mhn65536 · 2021-02-24T15:22:50Z

FF1Lib/LegendaryShops.cs

-			if (flags.LegendaryWhiteShop ?? false) result[3] = Allocate(ref slots, 4);//White
-			if (flags.LegendaryWeaponShop ?? false) result[0] = Allocate(ref slots, 3);//Weapon
-			if (flags.LegendaryItemShop ?? false) result[4] = Allocate(ref slots, 3);//Item
+			if (flags.LegendaryBlackShop ?? false)


Thats 5 lines inflated to 24.

yeah, this is due the the default rules and is up for discussion, in general I do think oneliners can be needlessly confusing when being more verbose only takes up more line -- its not like we need to ration lines. though I do agree that in specific cases oneliners can be better, as showcased here.

mhn65536 · 2021-02-24T16:04:05Z

I'd prefer it not do anything the VisualStudio "Format Document" command doesn't.

In the end, you want me to fix my own code as long as i'm around. If you change my code to the point, where i don't recognize it anymore, it'll severly hamper me.

Besides all theese formatting changes don't make the problems of the code go away. Spaghetti code will still be spaghetti code. And shitty code will at most become pretty shitty code.

But all this does, is ensure that neither you nor i know, what my code does. So as long as i'm around, I'd like to keep my code intact.

Steven-Biro · 2021-02-24T22:41:49Z

I'd prefer it not do anything the VisualStudio "Format Document" command doesn't.

This is the result of running dotnet format which is put out by the dotnet team, so while its not baked into the editor, its nearly just as standard. I'll look into making "Format Document" in Visual Studio run the dotnet formatter, or see if there is a separate command in Visual Studio that will do it for us.

In the end, you want me to fix my own code as long as i'm around. If you change my code to the point, where i don't recognize it anymore, it'll severly hamper me.

Besides all theese formatting changes don't make the problems of the code go away. Spaghetti code will still be spaghetti code. And shitty code will at most become pretty shitty code.

But all this does, is ensure that neither you nor i know, what my code does. So as long as i'm around, I'd like to keep my code intact.

This should be a one time migration, afterwards all the errors will show up in your editor and it will suggest how to fix it as well as block merging stuff that doesnt meet the style rules we end up agreeing on. Therefore, for everything after this is merged, it will be the exact code as you had it before that code is merged in.

Steven-Biro · 2021-02-24T23:27:25Z

this looks good to me
one question: what's the criteria for keeping lists' items on a single line or putting them on each of their line? going over everything it seems a bit random what list get expanded that way or not

If it's anything like prettier it switches to multiline when it doesn't fit within the line length you specify. If we don't explicitly set one somehow it's probably like 120 chars.

In my experience, forced style is worth the price of nobody being 100% happy with what it looks like, as long as IDE integrations can be tweaked to everybody's liking. I've been a format-on-save dude for a few years now, and just not even thinking about pressing enter or spacing things properly while coding, knowing that just saving once makes everything 'perfect' is a cool thing to get used to.

Unfortunately it appears the dotnet-format does not have prettier-style line breaking, and what we are seeing is the rule to preserve oneline vs when things are on multi-line, it just breaks it all into its own line. There is a github issue opened in mid 2019 which has the most ❤️ reactions and 👍 reactions out of all current issues, so there is demand and we might get that update in the future.

On the topic of line breaking as it currently stands though, we might need to disable the current rules as it does not enforce line length but rather forces you, for each collection, to have everything on one line or everything in mutliple lines, not ideal.

wildham0 · 2021-02-24T23:56:23Z

On the topic of line breaking as it currently stands though, we might need to disable the current rules as it does not enforce line length but rather forces you, for each collection, to have everything on one line or everything in mutliple lines, not ideal.

There just seems to be a few lists that aren't on only one line but get to keep their formatting, like in dialog.cs
(new List<MapLocation> { MapLocation.ConeriaCastleRoom1, MapLocation.ConeriaCastleRoom2, MapLocation.MirageTower1, MapLocation.SeaShrine1, MapLocation.SkyPalace1, MapLocation.TempleOfFiends1Room1, MapLocation.TempleOfFiends1Room2, MapLocation.TempleOfFiends1Room3, MapLocation.TempleOfFiends1Room4, MapLocation.CastleOrdeals1, MapLocation.TempleOfFiends2, MapLocation.EarthCave1, MapLocation.GurguVolcano1, MapLocation.IceCave1, MapLocation.MarshCave1 },

looking at it, maybe it's not applying the rule to lists inside lists.

Steven-Biro · 2021-02-25T00:10:56Z

On the topic of line breaking as it currently stands though, we might need to disable the current rules as it does not enforce line length but rather forces you, for each collection, to have everything on one line or everything in mutliple lines, not ideal.

There just seems to be a few lists that aren't on only one line but get to keep their formatting, like in dialog.cs
(new List<MapLocation> { MapLocation.ConeriaCastleRoom1, MapLocation.ConeriaCastleRoom2, MapLocation.MirageTower1, MapLocation.SeaShrine1, MapLocation.SkyPalace1, MapLocation.TempleOfFiends1Room1, MapLocation.TempleOfFiends1Room2, MapLocation.TempleOfFiends1Room3, MapLocation.TempleOfFiends1Room4, MapLocation.CastleOrdeals1, MapLocation.TempleOfFiends2, MapLocation.EarthCave1, MapLocation.GurguVolcano1, MapLocation.IceCave1, MapLocation.MarshCave1 },

looking at it, maybe it's not applying the rule to lists inside lists.

that is also entirely possible, Im open to suggestions on how you think we should handle this, try to follow one the the rules or disable them entirely and looking into a third party line length checker, or disable them and let PR reviewers catch that stuff.

mhn65536 · 2021-02-25T08:25:41Z

I'm still not entirely sure, what this will accomplish. I can see a lot of really badly formatted code in the project yes. But formatting it "nicely" will not make the code that much more readable and maintainable.

I mean, just look at what it did with enemizer. That's now a whooping 4800 lines instead of already unbearable 3750. Can anyone seriously argument, that he now understands what this code does any more than without theese changes? Besides beeing spaghetti code, the current formatting isn't actually bad. It's sure different, but consistent, clear and it conveys the individial constructs. It's just a really long spaghettified code piece that'll take a long time to get into, no matter what formatting.

Or take OverWorldMap. The changes it does, are superficial. A few extra lines here and there. But the algorithm of F/E Shuffle still has the same complexity.

Also the Formatter doesn't change bad variable and function names. It doesn't change that half our project is a partial class.

I will find theese formatting restrictions mostly annoying, especially if they prevent merges and commits. But they will neither make the code more readable nor more maintainable. I'm 100% sure of that.

Steven-Biro · 2021-03-17T22:18:25Z

This is temporarily on hold as I work on the FFRBot rewrite, but seeing as how most of the team wants standardized styling of some sort, I'll update the rules and redo this in a bit.

It will have scripts that can auto-format for those not using VS (or prefer to use a script) and will introduce a circleci check to ensure that the formatting is ok on future PRs

This will be a good first step in moving towards a slightly more organized development process including tests for adding to the changelog, adding tooltips, and (maybe, if this has enough traction) eventually running tests on FF1Lib itself

Steven-Biro added the DO NOT MERGE label Feb 21, 2021

Steven-Biro requested review from latzio, Entroper and wildham0 February 21, 2021 01:58

wildham0 reviewed Feb 21, 2021

View reviewed changes

FF1Lib/Dialogs.cs Outdated Show resolved Hide resolved

Steven-Biro added 2 commits February 20, 2021 22:05

turn on code style checking on build

a9e9615

stop git from changing our line endings

e2f7c6b

Steven-Biro force-pushed the sept/auto_format branch from ccd5b30 to a2c5d62 Compare February 21, 2021 05:01

Steven-Biro added 2 commits February 21, 2021 17:38

set style errors and build errors

a957fab

change some settings

eb5718e

Steven-Biro force-pushed the sept/auto_format branch from a2c5d62 to 50c734d Compare February 21, 2021 22:39

Steven-Biro added 5 commits February 21, 2021 17:56

manually fix switch case issues

2f9fec4

remove problematic rule 'use pattern matching' -- creates an if statm…

1d5d259

…ent that always results in false in spellcrafter, need to come back to this and see if it is the same end result

add another rule to .editorconfig

814c449

add rule to .editorconfig

b95a3c5

add comments in .editorconfig and fix a situation that was causing th…

7fdff01

…e autoformatter to crash

Steven-Biro force-pushed the sept/auto_format branch from 50c734d to 7ff3f41 Compare February 22, 2021 00:08

Steven-Biro added 6 commits February 23, 2021 22:34

Add Parentheses was having trouble -- ran it on its own

04b1c14

dotnet format -as

103cbb1

dotnet format -w

e2b7699

semi-manual fixes for things that wouldnt automatically get fixed

24a79dc

run dotnet format -as again

b79581a

more semi-manual fixes

0c7a64f

Steven-Biro force-pushed the sept/auto_format branch from 7ff3f41 to 0c7a64f Compare February 24, 2021 05:08

wildham0 reviewed Feb 24, 2021

View reviewed changes

mhn65536 reviewed Feb 24, 2021

View reviewed changes

wildham0 closed this Sep 14, 2024

wildham0 deleted the sept/auto_format branch September 14, 2024 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preliminary formatting changes #518

Preliminary formatting changes #518

Steven-Biro commented Feb 21, 2021 •

edited

Loading

wildham0 Feb 24, 2021

mhn65536 Feb 24, 2021

Steven-Biro Feb 24, 2021

Entroper Feb 28, 2021

wildham0 commented Feb 24, 2021

mikelattanziobluescape commented Feb 24, 2021 •

edited by latzio

Loading

mhn65536 Feb 24, 2021

Steven-Biro Feb 24, 2021

wildham0 Feb 24, 2021 •

edited

Loading

Steven-Biro Feb 25, 2021

mhn65536 Feb 25, 2021

Entroper Feb 28, 2021

mhn65536 Feb 24, 2021

Steven-Biro Feb 24, 2021

Entroper Feb 28, 2021

mhn65536 Feb 24, 2021

Steven-Biro Feb 24, 2021

mhn65536 Feb 25, 2021

mhn65536 Feb 24, 2021

mhn65536 Feb 24, 2021

Steven-Biro Feb 24, 2021

mhn65536 Feb 25, 2021

Entroper Feb 28, 2021

mhn65536 Feb 24, 2021

mhn65536 Feb 24, 2021

Steven-Biro Feb 24, 2021

mhn65536 commented Feb 24, 2021

Steven-Biro commented Feb 24, 2021

Steven-Biro commented Feb 24, 2021

wildham0 commented Feb 24, 2021

Steven-Biro commented Feb 25, 2021

mhn65536 commented Feb 25, 2021 •

edited

Loading

Steven-Biro commented Mar 17, 2021

Preliminary formatting changes #518

Preliminary formatting changes #518

Conversation

Steven-Biro commented Feb 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildham0 commented Feb 24, 2021

mikelattanziobluescape commented Feb 24, 2021 • edited by latzio Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildham0 Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhn65536 commented Feb 24, 2021

Steven-Biro commented Feb 24, 2021

Steven-Biro commented Feb 24, 2021

wildham0 commented Feb 24, 2021

Steven-Biro commented Feb 25, 2021

mhn65536 commented Feb 25, 2021 • edited Loading

Steven-Biro commented Mar 17, 2021

Steven-Biro commented Feb 21, 2021 •

edited

Loading

mikelattanziobluescape commented Feb 24, 2021 •

edited by latzio

Loading

wildham0 Feb 24, 2021 •

edited

Loading

mhn65536 commented Feb 25, 2021 •

edited

Loading