Skip to content

Conversation

@olivialynn
Copy link
Member

@olivialynn olivialynn commented Mar 24, 2023

Considerations for this PR:

No flags are specified in default run.

  • The ci.yml had no extra_flags specified in the default case (makes sense)
  • This lead to weird conditioning, such as pylint's step needing if: ${{ contains (matrix.copier_config.name, 'Base') }} to check that we wanted pylint in this run.
  • The alternative would have been checking that we did not have linter=black set in our extra flags
  • Neither is great (first breaks when we rename the default run; second breaks when we add or change alternative linters)
  • Maybe it would be better to always specify extra flags, even in the default run?
    • (How do we make sure they stay default?)
  • Maybe we can access the copier answers file once we generate the package?
    • (Can we do this elegantly?)

Considerations for the CI in general:

(will be copied over to the issue once this PR is closed)

This just tests the first available non-default option.

  • Some questions have more than two options, like mypy_type_checking including none, basic, and strict.
  • Do we want to exhaustively check each option?
  • Do we care which combinations are present (eg, pylint + BSD license + lfs disabled but also pylint + no license + lfs enabled), or is it sufficient to make sure we check each option at least once?

This only lets us run the existing CI checks with different options.

  • This does not technically do any checks that the options have worked as intended, beyond implicit assumptions like "we can only run the pytests on a module that has, in fact, been made" or "we can only run a pylint check if the options we picked lead us to actually install it."
  • So, this corrects our CI and lays the necessary foundation for further work, but it does not provide the thorough checks described in Introduce some integration tests for the template #77.

@olivialynn olivialynn changed the title Copy over from old branch to skip rebasing for just 1 file Updating CI to cover nondefault options Mar 24, 2023
@olivialynn olivialynn marked this pull request as ready for review March 24, 2023 12:48
Copy link
Collaborator

@drewoldag drewoldag left a comment

Choose a reason for hiding this comment

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

Thanks for doing this work @olivialynn. As it stands this is much better than it was before now that there are answers for all copier questions. And thanks for pointing out the confusion that arises around scenarios like the linter choice.

@olivialynn olivialynn merged commit cc9babf into main Mar 26, 2023
@olivialynn olivialynn deleted the issue/77/ci branch March 27, 2023 18:43
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