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

CLI Refactor #224

Merged
merged 12 commits into from
Sep 15, 2023
Merged

CLI Refactor #224

merged 12 commits into from
Sep 15, 2023

Conversation

NicEastvillage
Copy link
Contributor

This PR refactors our CLI

Notable changes for usage:

  • cgaal solver and cgaal global has been merged to cgaal check
    • The flag --global will make cgaal use the global algorithm (and conflicts with --search-strategy=)
  • Required arguments are now positional
    • E.g. cgaal check mymodel.lcgs myquery.atl instead of cgaal solver -m mymodel.lcgs -f myformula.atl
  • Some flags have been renamed
  • Some environment variables have been removed

Notable internal changes:

  • Argument parsing have been moved to args.rs
  • File loading has been moved to load.rs
  • Arguments are stored in CliOptions struct
  • Removed duplicate code caused by having to handle both LCGS vs JSON models
  • Reduced the use of std::exit, using Results instead

More refactoring is possible. E.g. update clap and use the new derive pattern (essentially the cli is generated from a struct like CliOptions), search strategy dispatching still cause a lot of duplicate code, but I will stop here before the PR gets too big.

@NicEastvillage NicEastvillage added the refactoring Rewriting or restructuring of existing features label Sep 14, 2023
Copy link
Member

@falkecarlsen falkecarlsen left a comment

Choose a reason for hiding this comment

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

Nice, some comments om SEMVER and args

cgaal-cli/src/args.rs Show resolved Hide resolved
cgaal-cli/src/args.rs Outdated Show resolved Hide resolved
cgaal-cli/Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Falke Carlsen <[email protected]>
Copy link
Member

@falkecarlsen falkecarlsen left a comment

Choose a reason for hiding this comment

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

:shipit:

@falkecarlsen falkecarlsen merged commit 5d5ae36 into main Sep 15, 2023
@falkecarlsen falkecarlsen deleted the cli-refactor branch September 15, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Rewriting or restructuring of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants