Namespacing CLI options

[sar009]

The __main__.py is blotted with a lot of CLI variables impacting readability and loose typing. So namespacing the CLI options under the class CliOptions.

[sar009]

fixing #859

[sungchun12]

Can you add a fuller description on your approach in this PR? I'll plan to review this next week when it's ready!

[sar009]

@sungchun12 I have updated the description please have a look.

[sungchun12]

Overall, I really like this approach and you did a great job of cleaning up the implementation emails and reducing kwargs bloat for function calls.

Once you update the data class for a pydantic class and update the relevant attribute calls, I'll give this another review and aim to merge this week!

[sungchun12]

thanks, good catch. You're reminding me that we need to add static type checking to this codebase

[sungchun12]

Seeing all this really puts into perspective just how many CLI options are available that need validation. Can you refactor this using pydantic?

Until we have static type checking in place, runtime type validation will serve this better

quick pseudo-code

from typing import Optional, Literal, Dict, Union, Tuple
from pydantic import BaseModel


class CliOptions(BaseModel):
    bisection_factor: int
    bisection_threshold: int
    table_write_limit: int
    database1: Union[str, Dict, None] = None
    table1: Optional[str] = None
    database2: Union[str, Dict, None] = None
    table2: Optional[str] = None
    key_columns: Tuple[str, ...] = ()
    update_column: Optional[str] = None
    columns: Tuple[str, ...] = ()
    limit: Optional[int] = None
    materialize_to_table: Optional[str] = None
    min_age: Optional[str] = None
    max_age: Optional[str] = None
    stats: bool = False
    debug: bool = False
    json_output: bool = False
    verbose: bool = False
    version: bool = False
    interactive: bool = False
    no_tracking: bool = False
    case_sensitive: bool = False
    assume_unique_key: bool = False
    sample_exclusive_rows: bool = False
    materialize_all_rows: bool = False
    threads: Union[None, int, Literal["serial"]] = None
    threads1: Optional[int] = None
    threads2: Optional[int] = None
    threaded: bool = False
    where: Optional[str] = None
    algorithm: Literal["auto", "joindiff", "hashdiff"] = "auto"
    conf: Optional[str] = None
    run: Optional[str] = None
    dbt: bool = False
    cloud: bool = False
    dbt_profiles_dir: Optional[str] = None
    dbt_project_dir: Optional[str] = None
    select: Optional[str] = None
    state: Optional[str] = None
    prod_database: Optional[str] = None
    prod_schema: Optional[str] = None
    __conf__: Optional[Dict] = None

[sungchun12]

You will have to change the attribute calls for the rest of the files as a result

[sungchun12]

pydantic will resolve this at runtime validation, but this custom error message is nice

[sar009]

@sungchun12 I have addressed the review comments please take a look

[sungchun12]

@sar009 thank you! I'll review later this week.

[sungchun12]

Mirror this PR for the full test suite to uncover any edge cases: #871

[sungchun12]

Overall, really good!

Just need new tests for these methods/functions given this codebase hasn't tested them before and we need to make sure the new args we're passing are doing exactly as expected.

• _apply_config
• apply_config_from_file
• _print_result
• _data_diff
• create_schema
• diff_schemas

[sungchun12]

Suggested change

	default=None,
	default=None,
	type=int,

[sar009]

https://github.com/datafold/data-diff/pull/860/files#diff-c4d05fd628acd83c42409afcbd38e6fca96261992bea7d774dd5d7ab598716a2R498

threads can be serial

[sungchun12]

need to verify how cli_options kwargs are being passed into this method

[sungchun12]

I think this makes sense instead of returning new_kw since cli_options stores config state now

[sungchun12]

This class is bloated and will become bloated over time, but I'm willing to accept that tradeoff for better runtime validation of flag types and simplification of kwargs** across method calls. Overall, this PR has opened my eyes that a major refactor needs to happen across lots of the codebase for simpler data flows, but in the short-term, this approach is worth it.

[github-actions]

This pull request has been marked as stale because it has been open for 60 days with no activity. If you would like the pull request to remain open, please comment on the pull request and it will be added to the triage queue. Otherwise, it will be closed in 7 days.

[github-actions]

Although we are closing this pull request as stale, it's not gone forever. PRs can be reopened if there is renewed community interest. Just add a comment and it will be reopened for triage.