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

chore(stacks-common): Remove unused imports and enable warning #5633

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

jbencin
Copy link
Contributor

@jbencin jbencin commented Jan 2, 2025

Description

Remove unused imports in stacks-common, and remove the line #![allow(unused_macros)] to enable unused import warnings for this crate

@jbencin jbencin requested a review from a team as a code owner January 2, 2025 04:01
@jbencin
Copy link
Contributor Author

jbencin commented Jan 2, 2025

There are a lot of issues if I run cargo build --all-features, so I'm moving this to draft

@jbencin jbencin marked this pull request as draft January 2, 2025 16:21
@jbencin
Copy link
Contributor Author

jbencin commented Jan 2, 2025

Looks like the serde feature flag is broken in develop, will fix or remove in a separate PR

For this PR will I will just fix slog_json

@jbencin jbencin marked this pull request as ready for review January 2, 2025 21:00
@jferrant
Copy link
Collaborator

jferrant commented Jan 2, 2025

Looks like the serde feature flag is broken in develop, will fix or remove in a separate PR

For this PR will I will just fix slog_json

You should pull develop cause I think we have fixed serde feature flag in the clippy changes PR.

jcnelson
jcnelson previously approved these changes Jan 3, 2025
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

LGTM as long as CI passes

@obycode
Copy link
Contributor

obycode commented Jan 3, 2025

LGTM - just fix the two new Clippy warnings.

@jbencin jbencin force-pushed the chore/remove-unused-imports-stacks-common branch from e28f957 to 10e6c6d Compare January 3, 2025 16:50
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

For future reference, after you have reviewers, it's easier for your reviewers if you update with a merge commit rather than a rebase and a force push. That way, reviewers can only look at changes since their last review.

@jbencin
Copy link
Contributor Author

jbencin commented Jan 3, 2025

For future reference, after you have reviewers, it's easier for your reviewers

Yeah I won't rebase if I have reviewer comments on the code, but GitHub also keeps track of what you've reviewed and I didn't think about that

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

👍

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.

4 participants