Skip to content

Conversation

paulfariello
Copy link

Add pre-commit configuration to ease development regarding CI checks

@roderickvd
Copy link
Member

I think it's a good idea. @photovoltex?

@photovoltex
Copy link
Member

I think nothing speaks against adding the config to the repo. Tho clippy needs to be invoke with --workspace and fmt with --all so that all projects are formatted and checked. Or is that already somewhere done? I don't really see what those id mentioned in the config does. Are there maybe any files missing that define those ids @paulfariello?

@paulfariello
Copy link
Author

I think nothing speaks against adding the config to the repo. Tho clippy needs to be invoke with --workspace and fmt with --all so that all projects are formatted and checked. Or is that already somewhere done? I don't really see what those id mentioned in the config does. Are there maybe any files missing that define those ids @paulfariello?

id are defined here: https://github.com/doublify/pre-commit-rust/blob/master/hooks.yaml

Concerning the --workspace option, it seems that it correspond to cargo check and documentation says:

By default, when no package selection options are given, the packages selected depend on the selected manifest file (based on the current working directory if --manifest-path is not given). If the manifest is the root of a workspace then the workspaces default members are selected, otherwise only the package defined by the manifest will be selected.

The default members of a workspace can be set explicitly with the workspace.default-members key in the root manifest. If this is not set, a virtual workspace will include all workspace members (equivalent to passing --workspace), and a non-virtual workspace will include only the root crate itself.

AFAICT current workspace configuration don't define any default-members, so all members should be selected. Which is confirmed by running cargo check and comparing with cargo check --workspace (same result as for clippy).

Do you want to include --workspace option in case we would change the current workspace definition?

As for fmt, pre-commit will run it only on individual files. Either only on modified files or on all files if pre-commit is run with --all argument.

Copy link
Member

@photovoltex photovoltex 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 clarifying that! I also seemed to have overlooked the link to a different repo in the config file. But seems good to me then.

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