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

Add lint, build, and test commands to justfile, add fmt and clippy to CI workflow #59

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

austin-denoble
Copy link
Contributor

Problem

@haruska had some good feedback around adding explicit build, test, and lint commands to the justfile in the repo. Currently there are only commands for dealing with code generation which is a little confusing.

Additionally, we haven't run cargo fmt and cargo clippy on the codebase yet, and have nothing enforcing its use.

Solution

  • Refactorjustfile: move all code generation under gen-* commands, add build, test, lint, and update commands to the file.
  • Update ci.yaml workflow to support both fmt and clippy. Reject if running either leads to issues.
  • Code changes for fmt and clippy.

Note: At the moment I've added #![allow(clippy::enum_variant_names)] and #![allow(clippy::empty_docs)] to the src/openapi/mod.rs file. There were some clippy issues in the generated code, and since we still need to refactor the entire generation flow in this repo, I just stuck the allow tags on top for now.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc)
  • None of the above: (explain here)

Test Plan

Make sure CI passes.

Copy link

@haruska haruska left a comment

Choose a reason for hiding this comment

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

Good improvement. Might also want to clean up the check-annotations on the build.

justfile Outdated
cargo build

# Build the OpenAPI and Protobuf definitions in the `codegen/apis` submodule
build-submodule-apis:
Copy link

Choose a reason for hiding this comment

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

i'd put this under a gen-* prefix as well

# Generate version file
generate-version:
echo "/// Pinecone API version\npub const API_VERSION: &str = \"{{api_version}}\";" > src/version.rs
test *tests: lint
Copy link

Choose a reason for hiding this comment

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

might want to add a comment for consistency.

@austin-denoble austin-denoble merged commit 2ccc8c1 into main Oct 1, 2024
4 checks passed
@austin-denoble austin-denoble deleted the adenoble/refactor-justfile-add-clippy-fmt branch October 1, 2024 20:56
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.

2 participants