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

Clippy linting and CI build/test. #30

Merged
merged 3 commits into from
Jan 3, 2023

Conversation

Xithrius
Copy link
Contributor

Hello!

I saw your post on the Rust community Discord, and thought I'd contribute this project.

The clippy group lints I've added are nursery and pedantic, which can be found here. There are some rules that I ignored that can be seen at the top of lib.rs.

I wasn't sure how to explain what the similarity sort function panics on, so I added a placeholder for the time being.

The CI builds and tests when there's a commit to main or any PR, and runs on the following operating systems and architectures:

windows-latest: x86_64-pc-windows-msvc
ubuntu-latest: x86_64-unknown-linux-gnu
macos-latest: x86_64-apple-darwin

Happy holidays!

@ParthJadhav
Copy link
Owner

Hello there @Xithrius ,

Firstly thanks for making this contribution in a holiday season 😄

Few questions:

  1. Why do we need to build it for all the three platforms when it's gonna directly be downloaded from the cargo. Is it that, If I compile or publish from MacOS the other platforms won't be able to use the crate? Also it doesn't build on ARM for MacOS would that cause any issues for users on M Series Macs ?

  2. The Build command in the workflow... does it need to have -r for the release version or it's okay not to have it.

  3. The lint and formatting runs after the build? Shouldn't it run before so we catch certain ill practices before hand?

I'm new to Rust development. Please excuse dumb questions.

@Xithrius
Copy link
Contributor Author

  1. Building for all 3 platforms is just to make sure that there's no broken or extra dependencies needed. For instance, one of my applications broke when open-ssh needed something more for macos. If you build for all 3 and it works, publishing from any of them will work for the rest.
  2. It's ok not to have it.
  3. If there's a syntax error that the build command catches or something like that, then it won't be that useful to run the lint in my opinion.

@ParthJadhav
Copy link
Owner

Ohh, Intresting @Xithrius . Got it. I'll test it once and we can merge it.

@ParthJadhav
Copy link
Owner

ParthJadhav commented Jan 2, 2023

One more thing @Xithrius , The builds for different platforms... How are they gonna be used ? I mean

Would that have any effect on the cargo publish command. or is it just to test if everything works properly?

Copy link
Owner

@ParthJadhav ParthJadhav left a comment

Choose a reason for hiding this comment

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

Just left few comments

@Xithrius
Copy link
Contributor Author

Xithrius commented Jan 2, 2023

One more thing @Xithrius , The builds for different platforms... How are they gonna be used ? I mean

Would that have any effect on the cargo publish command. or is it just to test if everything works properly?

@ParthJadhav Correct, it is only for testing. cargo publish won't be effected.

@ParthJadhav
Copy link
Owner

Got it @Xithrius , Will merge it 🚀 Thanks for the enhancements.

@ParthJadhav ParthJadhav merged commit 2403afb into ParthJadhav:master Jan 3, 2023
@Xithrius Xithrius deleted the feature/ci-and-clippy branch January 3, 2023 04:57
@Xithrius Xithrius restored the feature/ci-and-clippy branch January 3, 2023 04:58
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