Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better list diffing2 #79

Merged
merged 17 commits into from Nov 18, 2016
Merged

Better list diffing2 #79

merged 17 commits into from Nov 18, 2016

Conversation

bluebird75
Copy link
Owner

Cleaned up version of the list diff analysis

@bluebird75 bluebird75 mentioned this pull request Nov 10, 2016
-- line = line:sub(1,idxStart-1)..source..line:sub(idxEnd+1)
-- string.gsub( line, dest, source )
if verbose then
print('Result: '..line )
print('Result : '..line )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That whitespace looks weird, is it really supposed to be there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I got it. It brings the line's prefix up to the same length as 'Modifying line: ' further up, for aligment / easier comparison.

@@ -1036,6 +1029,7 @@ local function _is_table_equals(actual, expected, recursions)
return true
end
M.private._is_table_equals = _is_table_equals
local is_equals = _is_table_equals
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer singular here in the function name, i.e. is_equal.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not...

@bluebird75
Copy link
Owner Author

When I use a local, it does not work. I get the following error message :

d:\work\luaunit\luaunit-git\luaunit2>lua run_unit_tests.lua
is_equalsfunction: 0083B6F8`
.....................................................................................is_equalsnil
F....................................is_equalsnil
E......................is_equalsnil
E...
Failed tests:

1) TestLuaUnitErrorMsg.test_printTableWithRef

.\test\test_luaunit.lua:1937: Error message does not contain: "lists <table: "

Error message received: ".\luaunit.lua:661: attempt to call global 'is_equals' (a nil value)"


stack traceback:
    .\test\test_luaunit.lua:1937: in function 'TestLuaUnitErrorMsg.test_printTableWithRef'


2) TestLuaUnitUtilities.test_diffAnalysisThreshold

.\luaunit.lua:661: attempt to call global 'is_equals' (a nil value)

stack traceback:
    .\test\test_luaunit.lua:253: in function 'TestLuaUnitUtilities.test_diffAnalysisThreshold'


3) TestLuaUnitUtilities.test_suitableForMismatchFormatting

.\luaunit.lua:661: attempt to call global 'is_equals' (a nil value)

stack traceback:
    .\test\test_luaunit.lua:246: in function 'TestLuaUnitUtilities.test_suitableForMismatchFormatting'


Ran 149 tests in 0.024 seconds, 146 successes, 1 failure, 2 errors

The spurious prints qre because I added the extra lines to debug the situation :

lines 658-661 :

i = 1
print( 'is_equals'..tostring(is_equals ) )
while i <= longest do
     if not is_equals(ta[i], tb[i]) then

This is the is_equals which triggers the error.

lines 1044-1047 :

M.private._is_table_equals = _is_table_equals
local is_equals = _is_table_equals
print( 'is_equals'..tostring(is_equals ) )

@bluebird75
Copy link
Owner Author

The build fails because lua does not have the same representation of numbers on windows and linux...

Annoying for test based on textual representations :-)

@n1tehawk
Copy link
Contributor

Okay, I now see where the problem lies: Since your new routines are further up in the source code, this is a case of "use before declare" - leading to those errors.

There are two possible solutions: Either a) move the entire _is_table_equals() block/logic further up (or your list formatting further down); or b) declare something like

local is_equals -- alias for _is_table_equals() function, gets assigned below

near the top of the file (e.g. under "general utility functions"), and then do a simple is_equals = _is_table_equals after the function has been declared.

Regards, NiteHawk

@n1tehawk
Copy link
Contributor

The build fails because lua does not have the same representation of numbers on windows and linux...

We have encountered and worked around number formatting issues before, e.g. test/test_luaunit.lua#L1671-L1675.

Maybe this would be a good opportunity to tackle the problem at the prettystr() level, eliminating the differences between Lua versions?

@n1tehawk
Copy link
Contributor

n1tehawk@e0d1d18 illustrates what I have in mind...

@coveralls
Copy link

coveralls commented Nov 12, 2016

Coverage Status

Coverage decreased (-4.3%) to 95.514% when pulling 5149065 on better-list-diffing2 into d1a9ddc on master.

@bluebird75
Copy link
Owner Author

That's actually a good idea ! We can do it after merging in this branch.

You want to take care of it ?

Apart from that, any further comments on this branch ?

@coveralls
Copy link

coveralls commented Nov 13, 2016

Coverage Status

Coverage increased (+0.01%) to 99.838% when pulling f304be6 on better-list-diffing2 into d1a9ddc on master.

@n1tehawk
Copy link
Contributor

That's actually a good idea ! We can do it after merging in this branch.
You want to take care of it ?

Sure, I can do that.

Apart from that, any further comments on this branch ?

Not really. I've noted a few smaller things here and there, but it's a bit hard to keep track of this PR in its entirety (as it needs some rebasing onto the current master by now). I think you should merge it first, if specific issues come up after that, we can still improve on / fix them later.

If a more general luacheck is giving you troubles (makes you struggle with Lua syntax), I'll gladly help with that too.

Regards, NiteHawk

@bluebird75 bluebird75 merged commit 41927da into master Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants