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

Appease clippy and add to CI #10

Merged
merged 3 commits into from
Apr 5, 2022
Merged

Appease clippy and add to CI #10

merged 3 commits into from
Apr 5, 2022

Conversation

Property404
Copy link
Contributor

Seems like Clippy hasn't been run

Copy link
Owner

@Samyak2 Samyak2 left a comment

Choose a reason for hiding this comment

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

Thank you for this! I was planning to address clippy at some point, but I appreciate this.

Looks like one clippy hint was missed (or deliberately ignored for the sake of not breaking the API?):

warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices
   --> src/tui.rs:345:44
    |
345 |     pub fn display_words(&mut self, words: &Vec<String>) -> MaybeError<Vec<Text>> {
    |                                            ^^^^^^^^^^^^ help: change this to: `&[String]`
    |
    = note: `#[warn(clippy::ptr_arg)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

Also, what do you think about adding clippy as step in CI?

@Property404 Property404 changed the title Appease clippy lints Appease clippy and add to CI Apr 4, 2022
Copy link
Owner

@Samyak2 Samyak2 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for contributing!

@Samyak2 Samyak2 merged commit 01ba676 into Samyak2:main Apr 5, 2022
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