Allow to pass various models to set_llm.

[krystian8207]

Closes #59

[maciekbanas]

Finished review. Basic logic looks good. I have two questions regarding get_gitlab_pat() handler and failure in creating turn object in process_content() function when testing.

[maciekbanas]

I am not sure about this approach. Basically GitStats looks by itself for best-fit access key when token parameter is set to NULL (searches through all keys that start with GITLAB_*). Now, if you leave token parameter empty, tests will pass if you have any key starting with GITLAB_* that gives access to public GitLab API. If you will use approach with get_gitlab_pat(), test will fail if user does not have explicit GITLAB_PAT key as one giving access to public GitLab.

My suggestion would be to create token parameter on the level of set_*_repos() function which value would be then passed to GitStats set_*_host() function (here is issue for that: #58). By default it would stay NULL. For tests we could define it e.g. as GITLAB_PAT_PUBLIC. We could still leave it empty and pack it into withr the way I do in GitStats to test pulling default token:

test_that("When empty token for GitLab, GitStats pulls default token", {
  skip_on_cran()
  expect_snapshot(
    withr::with_envvar(new = c("GITLAB_PAT" = Sys.getenv("GITLAB_PAT_PUBLIC")), {
      test_gitstats <- create_gitstats() %>%
        set_gitlab_host(
          orgs = "mbtests"
        )
    })
  )
})

[krystian8207]

Good point, I wasn't aware of this functionality of GitStats.
We agreed to add skipping tests directly in the GitStats-related methods.

[maciekbanas]

Tests locally fail here, as turn object is NULL. It is created with last_turn() method, not sure if it should be somehow mocked?

[krystian8207]

It happened due to overwriting elmer:::Chat (indirectly by modification of ChatMocked).
Fixed it by using inherited R6 Class.

[maciekbanas]

The example from README also fails on turn object. Maybe something has been changed in elmer recently?

[maciekbanas]

Now works great. Thank you.

[kalimu]

Could you explain why we need to add this [integration test] prefixes to our tests?

[krystian8207]

It's optional, just wanted to distinct the ones we're supposed to run with external keys.
I can revert it if you want.

[kalimu]

Got it. I was wondering if this is for some automatic processing I'm not aware about.
So I would remove those prefixes, as they seem to be nothing more then flag comments that can easily be out of sync with the code. Instead (we don't have to do that in this PR though) I would add a helper function in the utils like:
skip_if_integration_tests_should_be_skipped() (or sth shorter but that is what I found most informative so far)
This function could be added at the beginning of all integration tests. Inside the function can have skip_on_cran() and additionally it can skip test if some environmental variable is set to TRUE, like GITAI_SKIP_INTEGRATION_TESTS.
It would be easy to switch off all integration tests with that while developing locally if needed or inside CI/CD.

[krystian8207]

Good point, gonna remove the prefixes then.