Changes made for table iteration

[rabelmervin]

Description

Initially the tables used a simple string method of matching. Now its updated to iteration method that iterates over the tables. previous error with the formatting also rectified.

[rabelmervin]

hi @tedivm black formatting issue fixed ! please review it

[tedivm]

We still need tests for the new functionality.

[tedivm]

For testing basically copy the test_graph_with_exclusion and test_graph_with_inclusion functions (https://github.com/tedivm/paracelsus/blob/main/tests/test_cli.py#L42) so you have test_graph_with_exclusion_regex and test_graph_with_inclusion_regex.

Then tweak the command line arguments to use some regex patterns, before finally testing that the appropriate tables are present (or not present, as the case may be).

[rabelmervin]

Sure @tedivm thanks for your information 😊

[rabelmervin]

Hi @tedivm test was successfully passed can i make a pr now ?

[tedivm]

Just push the new tests to this branch and this branch will automatically update and I can review.

[tedivm]

I provided some comments to help improve the tests.

[tedivm]

These tests don't make sense- I don't know where these strings are coming from. The goal here is to test that the proper columns exist, but the excluded ones do not.

In this case we have three tables- comments, users, and posts. Since we're trying to include the pattern ^com.* we expect the comments table to be in the diagram but not the other two.

assert "comments {" in result.stdout
assert "users {" not in result.stdout
assert "post {" not in result.stdout

[rabelmervin]

I apologise @tedivm I'll make changes !

[tedivm]

No worries! I appreciate you taking this on!

[tedivm]

In this case we have three tables- comments, users, and posts. Since we're trying to exclude the pattern ^pos.* we expect the comments and users table to be in the diagram but not the posts one.

assert "comments {" in result.stdout
assert "users {" in result.stdout
assert "post {" not in result.stdout

[rabelmervin]

hi @tedivm I have changed that useless strings, used regex pattern and tests passed. could you please tell me if there is any mistakes ?

[tedivm]

This should remain ^com.*, otherwise you aren't testing the regex.

[tedivm]

I'm pretty sure this should be ^pos.*

[tedivm]

It looks like when you updated the tests you removed the regex from them, but the whole point of these tests is to make sure the regex is actually used and works as expected.

[tedivm]

Don't do this- use the existing test database, it works perfectly for this use case.

Also imports should never be here, always at the top of the file.

[rabelmervin]

Sorry @tedivm , Now i made changes can you please review it ?

[tedivm]

Okay, so the tests themselves look good now. They're testing what we want to test.

However, it looks like one of the tests is failing.

    def test_graph_with_inclusion_regex(package_path: Path):
        result = runner.invoke(
            app,
            [
                "graph",
                "example.base:Base",
                "--import-module",
                "example.models",
                "--python-dir",
                str(package_path),
                "--include-tables",
                "^com.*",
            ],
        )
        assert result.exit_code == 0
>       assert "comments {" in result.stdout
E       AssertionError: assert 'comments {' in 'erDiagram\n\n'
E        +  where 'erDiagram\n\n' = <Result okay>.stdout

tests/test_cli.py:162: AssertionError

This error is showing that the comments table isn't in the diagram, even though we expect it to be. You don't need to change the tests to fix this, but look at the code itself with the changes. I'm going to put some inline comments to show what I think is happening.

[tedivm]

Github won't let me make the comment inline, but on line 97 of the paracelsus/graph.py file you likely want to change return include_tables to return included.

On line 92 you should also change include_tables to included.

I think with those two changes tests will pass.

[tedivm]

Actually I was wrong about line 92, it's probably just line 97 that needs the change.

[rabelmervin]

Good morning @tedivm, I have made changes include_tables to included

[tedivm]

It all works now! Thank you so much for contributing and working through the review process.

[rabelmervin]

Thank you @tedivm for merging my PR and supporting me during the process. I really value the collaboration and would love to stay connected. Looking forward to learning from you and contributing more in the future.