GLua eradication by oldmud0 · Pull Request #1253 · wiremod/wire

oldmud0 · 2016-11-04T04:01:06Z

The purpose of this PR is to eradicate all occurrences of unofficial Lua syntax. All of it. This is an ongoing PR, and it will not merge until luacheck passes with no syntax errors across the entire source code.

// -> --, != -> ~=, ! -> not s/if\s*?\(\s*(.*?)\s*\)\s*?then/if \1 then/ (removes extra parentheses in if-then)

# Conflicts: # lua/entities/gmod_wire_hudindicator/init.lua # lua/weapons/gmod_tool/stools/wire_namer.lua

Divran · 2016-11-04T10:20:43Z

I see you're even erasing brackets around if statements. That's not unofficial lua syntax, just sayin

EDIT: you merged "master" into glua-eradication? was that intentional?

AbigailBuccaneer · 2016-11-04T11:23:11Z

 	Very useful for searching algorithms
 	Used by custom spawn menu search & gate tool search, for example
-	Credits go to: http://lua-users.org/lists/lua-l/2009-07/msg00461.html
+	Credits go to: http:-- lua-users.org/lists/lua-l/2009-07/msg00461.html


Please fix this and any other instances of incorrect replacements.

oldmud0 · 2016-11-04T12:02:28Z

The parentheses thing I considered part of the "unofficial syntax" because Lua neither requires nor condones parentheses around the entire condition. It's annoying, it's not part of the manual of style, and it's not in new code.

I had to merge against master to fix a conflict with master branch that would not have allowed merging (like that's going to happen anytime soon).

I was expecting someone to point out some mistakes I inevitably made trying to fix literally everything. Thank you for reviewing.

TomyLobo · 2016-11-05T10:10:18Z

 	self.BaseClass.Think(self)

-	if ( self:GetOn() ) then
+	if  self:GetOn()  then


there are extra spaces here

TomyLobo · 2016-11-05T10:11:59Z

 	self.BaseClass.Think(self)

-	// If we were undiskged, reset the disk and socket to accept new ones.
+	-- If we were undiskged, reset the disk and socket to accept new ones.


typo "undiskged"

TomyLobo · 2016-11-05T10:15:55Z

+	if not self.Disk then return end
 	local disk = self.Disk
-	if (self.Command[8] ~= 0) then
+	if self.Command[8] ~= 0 then


wow someone should really replace those numbers by constants with telling names

not as part of this PR, of course

TomyLobo

I've pointed out a number of things that need to change abefore we can accept this PR as well as some things that might be interesting as part of separate PRs.

For some of the required changes, I wrote a script while doing the review. I'll post a PR on your branch as soon as I flesh out that script.

The comments corresponding to those changes should start disappearing as soon as you accept that PR.

TomyLobo · 2016-11-05T10:19:10Z

-  if (self.Memory[2047] ~= 0) && (self.CharX >= 0) && (self.CharX < 30) &&
-                                 (self.CharY >= 0) && (self.CharY < 18) then
+  if (self.Memory[2047] ~= 0) and (self.CharX >= 0) and (self.CharX < 30) and
+                                 (self.CharY >= 0) and (self.CharY < 18) then


The indentation seems off here. might as well fix it.

TomyLobo · 2016-11-05T10:20:56Z

-	// If we have no plug in us
-	if (not self.MyPlug) or (not self.MyPlug:IsValid()) then
+	-- If we have no plug in us
+	if not self.MyPlug) or (not self.MyPlug:IsValid() then


TomyLobo · 2016-11-05T10:21:44Z

-			// If we find a plug, try to attach it to us
-			if ( plug:IsValid() && plug:GetClass() == "gmod_wire_dataplug" ) then
+			-- If we find a plug, try to attach it to us
+			if  plug:IsValid() && plug:GetClass() == "gmod_wire_dataplug"  then


there are extra spaces here

TomyLobo · 2016-11-05T10:23:10Z

 				end
 			end
-		elseif Address == 1048575 then -- CLK
+		-- elseif Address == 1048575 then -- CLK


intentional?

~~Definitely not. I tested the comments find/replace well and it never did this.~~ Yes, this is intentional. Leaving this elseif triggers an error in luacheck. See the following line.

TomyLobo · 2016-11-05T10:26:57Z

 			SetMaterial( Mat )
 		elseif isentity(Mat) then
-			if (!Mat:IsValid() or !Mat.GPU or !Mat.GPU.RT) then NoTexture() return end
+			if not (Mat:IsValid() and Mat.GPU and Mat.GPU.RT) then NoTexture() return end


TomyLobo · 2016-11-05T11:25:06Z


 	for k, v in pairs(WIREENT) do
-		if ((Entity[k] == nil) or (k == "_Overrides_WireMapInterfaceEnt") or (k == "_Added_WireMapInterfaceEnt") or (k == "_Settings_WireMapInterfaceEnt")) then
+		if Entity[k] == nil) or (k == "_Overrides_WireMapInterfaceEnt") or (k == "_Added_WireMapInterfaceEnt") or (k == "_Settings_WireMapInterfaceEnt" then


TomyLobo · 2016-11-05T11:46:25Z

 			--Msg("========BeamVars leaving NormalOpMode | "..DelayedUpdatesNum.."\n")
 			NormalOpMode = false
-		elseif (!NormalOpMode) and (DelayedUpdatesNum < 50)  then
+		elseif (not NormalOpMode) and (DelayedUpdatesNum < 50)  then


there are exta spaces here

TomyLobo · 2016-11-05T12:45:17Z



-/*---------------------------------------------------------
+--[[---------------------------------------------------------


extra dashes here and further down a couple of times

TomyLobo · 2016-11-05T13:02:28Z

 		if ent:GetClass() == "gmod_wire_trigger" then

-			-- http:--youtu.be/RTR1ny0O_io
+			-- http://youtu.be/RTR1ny0O_io


another broken url

Meant to fix this, but you beat me to it.

TomyLobo · 2016-11-05T13:09:50Z

+-- s_mode: Tool_mode, same as the old tool lua file name, minus the "wire_" part
+-- s_name: Proper name for the tool
+-- s_class: For tools that make a device. Should begin with "gmod_wire_". Can be nil if not using WireToolObj.LeftClick or WireToolSetup.BaseLang
+-- f_toolmakeent: Server side function for making the tools device. Can be nil if not using WireToolObj.LeftClick


Can you undo this change? I think the extra indentation here had a purpose.

If you see the previous occurrence of this, you will see that the in-comment indentation is a bit inconsistent. This one only has space of indentation whereas the previous one had two.

TomyLobo · 2016-11-05T15:51:17Z

still working on the script.
turns out determining the difference between code and prose is hard.

EDIT:
Done, oldmud0#1

oldmud0 · 2016-11-06T02:01:21Z

Thanks for your contributions. I wanted to do some things too but it looks like you have beat me to them. I'll do the second pass now.

UPDATE: We have a somewhat significant problem. continue is also part of GLua syntax but the fix is nontrivial.

oldmud0

On the whitespace fixes: This is purely to try to get the build to pass, not because I am actually some sort of whitespace purist.

Unnecessary parentheses in if statements were not removed.

TomyLobo · 2016-11-06T22:40:21Z


 local string = string -- optimization

-/******************************************************************************/


maybe reinstate those as lines of dashes?

I did, but it looks like I accidentally didn't push it

TomyLobo · 2016-11-06T22:44:48Z

 end

-/* --- File Read --- */
+--[[ --- File Read --- ]]


could have just removed the /* */ :D

You're right, that would have looked a lot neater.

oldmud0 · 2016-11-07T02:47:28Z

Okay, so I have found another problem. When e2functions are defined, the declaration is considered an error by luacheck, so basically any file defining any E2 functions is an error (afaik luacheck stops at the first error on a file).

TomyLobo · 2016-11-07T18:31:16Z

re e2function: Can you maybe preprocess them with a regex before piping them into luacheck?
I could give you a sed script that turns e2functions into regular functions, probably even with all the parameter names intact.

re continue: maybe you can do the same here... replace it by break.

oldmud0 · 2017-11-11T19:35:31Z

As a conclusion, the effort was noble, but is not easily doable because it involves touching some really old code that is known to work and also reflowing code that depends on continue.

If I wanted to do the eradication, I'd probably just start over, scout for common style problems, do the heavy lifting purely with regex/awk, and then do the final pass by hand. I made too many mistakes trying to do this with semi-manual regex, most especially just missing things by going too fast.

oldmud0 added 3 commits October 30, 2016 00:03

GLua eradication - 1st pass (effects, entities, weapons)

ee59f48

// -> --, != -> ~=, ! -> not s/if\s*?\(\s*(.*?)\s*\)\s*?then/if \1 then/ (removes extra parentheses in if-then)

GLua eradication - 1st pass (core)

9704975

Merge branch 'master' into glua-eradication

8001abd

# Conflicts: # lua/entities/gmod_wire_hudindicator/init.lua # lua/weapons/gmod_tool/stools/wire_namer.lua

AbigailBuccaneer requested changes Nov 4, 2016

View reviewed changes

AbigailBuccaneer assigned oldmud0 Nov 4, 2016

TomyLobo reviewed Nov 5, 2016

View reviewed changes

TomyLobo requested changes Nov 5, 2016

View reviewed changes

TomyLobo mentioned this pull request Nov 5, 2016

My Adjustments oldmud0/wire#1

Merged

TomyLobo added 5 commits November 5, 2016 19:57

Fix some URLs

6cbc9b0

Remove useless spaces in if statements

bb3d892

Remove spaces in front of a bunch of function argument lists

4259248

Move comment to the appropriate line

6c28d53

Remove spaces in front of commented-out code

9dab832

oldmud0 added 3 commits November 5, 2016 21:09

GLua eradication - 2nd pass (TomyLobo's review)

55664b3

GLua eradication - 2nd pass (luacheck errors)

9566f30

Whitespace fixes

62756b6

oldmud0 commented Nov 6, 2016

View reviewed changes

oldmud0 added 2 commits November 6, 2016 16:18

GLua eradication - 3rd pass (deep luacheck)

3ad12f6

Unnecessary parentheses in if statements were not removed.

Corrected the long horizontal lines

e49b341

TomyLobo reviewed Nov 6, 2016

View reviewed changes

Remove some more spaces in front of commented-out code

f00b784

TomyLobo added 2 commits November 7, 2016 16:59

Fix typo

af9c453

Fix broken dashed lines

ca2a0e4

AbigailBuccaneer closed this Nov 11, 2017



		/*---------------------------------------------------------
		--[[---------------------------------------------------------


		local string = string -- optimization

		/******************************************************************************/

Uh oh!

Conversation

oldmud0 commented Nov 4, 2016

Uh oh!

Divran commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oldmud0 commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomyLobo Nov 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomyLobo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oldmud0 Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomyLobo commented Nov 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oldmud0 commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oldmud0 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oldmud0 Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oldmud0 commented Nov 7, 2016

Uh oh!

TomyLobo commented Nov 7, 2016

Uh oh!

oldmud0 commented Nov 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Divran commented Nov 4, 2016 •

edited

Loading

TomyLobo Nov 5, 2016 •

edited

Loading

TomyLobo left a comment •

edited

Loading

oldmud0 Nov 6, 2016 •

edited

Loading

TomyLobo commented Nov 5, 2016 •

edited

Loading

oldmud0 commented Nov 6, 2016 •

edited

Loading

oldmud0 left a comment •

edited

Loading

oldmud0 Nov 7, 2016 •

edited

Loading