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

add expect functions for matching table values #44

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

willcurry
Copy link

No description provided.

function expectations.haveValuesMatch( comparison )
local match_count = 0
for k, v in pairs(subject) do
if table.HasValue(comparison, v) then
Copy link

Choose a reason for hiding this comment

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

table.HasValue() has a O(n) run time, so your check runs similar to O(n^2). You can convert this check behavior to O(n) by converting one of the tables to have the values you are going to check for as keys (outside the for loop). Use the index operator for the check. if valuesToCheck[myValueToCheck] then ... end

https://wiki.facepunch.com/gmod/table.HasValue

Copy link
Author

Choose a reason for hiding this comment

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

That wouldn't work as you can't have duplicate keys but you can have duplicate values.

Copy link

Choose a reason for hiding this comment

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

I see, after those fixes it makes sense to me now.

@brandonsturgeon brandonsturgeon added the enhancement New feature or request label Feb 22, 2023
@brandonsturgeon
Copy link
Member

Hello! Thanks for the PR

I'm swamped with work at the moment, but this is top-prio for me when I free up!

@brandonsturgeon
Copy link
Member

brandonsturgeon commented Mar 1, 2023

Thanks for the PR!

One thing I learned when doing deep dives on other testing frameworks was to be careful about bloating the base library.
To me, that means when reviewing additions like new expectations, mocking features, etc. - the question should be "Why?" rather than "Why not?".
(The idea being that other parties can create and maintain more specific [or perhaps broader] libraries to extend the functionality of the project)

To that end, I wanted to ask you about the use case for this new expectation.

To accomplish this same thing with the current set of expectations, I might do something like this:

expect( tbl[key1] ).to.equal( value1 )
expect( tbl[key2] ).to.equal( value2 )
expect( tbl[key3] ).to.equal( value3 )
expect( tbl[key4] ).to.equal( value4 )

Or perhaps if I had a much larger table, I might do something like this (though I generally shy away from metaprogramming)

for key, value in pairs( expected ) do
    expect( tbl[key] ).to.equal( value )
end

Doing it that way could be cumbersome for very large tables, granted, but this way you'd know exactly which key (albeit only one at a time) failed the expectation.

With your code, I believe you'd only see the table signature in the error message (which could be changed).

What's your reasoning or use case for such an expectation? Was there a specific problem you ran into that this helped alleviate?

Please don't get me wrong, I'm elated that you took the time to fork, read, and suggest additions to the code (seriously, thank you, it means a lot 🙏).
I think a project like this will have the highest chance of success when more people get involved.
I would be remiss, however, if I didn't do my due diligence as a maintainer.

Anyway, looking forward to hearing back from you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants