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

Rework solution api #71

Merged

Conversation

ellnix
Copy link
Collaborator

@ellnix ellnix commented May 1, 2024

  • Renamed the run function to lazy_run
  • Added tests for lazy_run
    • While adding them I realized that App may need to be in the library and that these tests cannot run in CI because they depend on the clash dir, which inspired Test "framework" #72
    • I made another commit to delete the tests which I will revert when we have a way to run these tests in CI
  • Made all the params and return types as generic as possible.
  • Removed SuiteRun and replaced it with a call to map() 🤦
  • Refactored App.run slightly

Regarding App

I suggest

  • Moving App to lib
  • Moving all the helper functions (functions that do not have a corresponding command) into App
  • Removing the functions that have commands corresponding to them from App and just having them as regular functions in main.rs (or as part of a Cli type)
  • Making the App constructor to take a single package_name parameter:
    let app = App::new("dev.CoCtus.coctus");

This would then separate the responsibility between calling the commands and the helper functions which we could use in tests. It would also prepare for #61 by allowing configuration to be stored in App and be used by users of the lib.

@ellnix ellnix marked this pull request as ready for review May 1, 2024 14:29
Copy link
Owner

@Andriamanitra Andriamanitra left a comment

Choose a reason for hiding this comment

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

I don't know if the unable_to_run and get_result functions are particularly useful as they can only ever be called from one place but it's whatever. The behavior change in --timeout needs fixing though.

src/main.rs Outdated Show resolved Hide resolved
src/solution.rs Outdated Show resolved Hide resolved
@ellnix
Copy link
Collaborator Author

ellnix commented May 1, 2024

I don't know if the unable_to_run and get_result functions are particularly useful as they can only ever be called from one place but it's whatever.

The purpose was more to make each function easier to skim + group and document what those lines of code were doing.

The behavior change in --timeout needs fixing though.

Done, sorry about that.

@ellnix ellnix requested a review from Andriamanitra May 1, 2024 18:08
@Andriamanitra Andriamanitra merged commit 8213cd6 into Andriamanitra:separate-domain-logic May 2, 2024
@ellnix ellnix mentioned this pull request May 2, 2024
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.

3 participants