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

Added better handling for empty tables #152

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Conversation

miversen33
Copy link
Contributor

Addresses #146

Note, I don't really know how to write a test to properly address the change in logic applied here. However I did run make and all tests passed.

That said, its worth considering if this is how you want to address this issue or not :)

@olimorris
Copy link
Owner

This is a great spot and exactly what is happening. Thank you.

Can you add your test from #146 as well?

@miversen33
Copy link
Contributor Author

This is a great spot and exactly what is happening. Thank you.

Can you add your test from #146 as well?

Technically the test I provide there fails as the logic in that test is ever so slightly different from what I am actually doing. That said, I will see if I toss something akin to it in here for you :)

@miversen33
Copy link
Contributor Author

@olimorris I have added a modified version of the test I recommended earlier.

That said, I really don't like it. The logic added is actually in init not dirs_match, but there is no test (from what I saw) for init.allowed_dir so what I added is my best shot at it.

The test is not really clear on what its doing so I added some comments. Tests shouldn't need comments though :(

It works but I would advise you peek at it and make it how you want

@olimorris olimorris merged commit c9c11ee into olimorris:main Aug 30, 2024
5 checks passed
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.

2 participants