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

new rust clippy #1292

Merged
merged 2 commits into from
Jan 27, 2023
Merged

new rust clippy #1292

merged 2 commits into from
Jan 27, 2023

Conversation

adrian-kong
Copy link
Contributor

@adrian-kong adrian-kong requested review from notoriaga, silverjam and a team as code owners January 27, 2023 00:10
@adrian-kong adrian-kong enabled auto-merge (squash) January 27, 2023 00:14
Copy link
Contributor

@pcrumley pcrumley left a comment

Choose a reason for hiding this comment

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

did you just run cargo clippy --fix?

because it seems like if that was supposed to be it i don't know why it added some bin files.
Screenshot 2023-01-26 at 4 23 55 PM

Seems like there are others too.

i also don't know why a .rs file would be a binary.

@adrian-kong
Copy link
Contributor Author

not sure why its a BIN file but all that was changed was the generator file (to enforce make gen-rust have same clippys) and cargo clippy --all-targets --all-features --fix

@pcrumley
Copy link
Contributor

Screenshot 2023-01-26 at 5 17 44 PM

Those files are being marked as binary because they contain raw null bytes in them. It also is occuring in one of the places where the lint was being hit. I think we should fix it and have the null bytes be written as a binary string. i.e. in this case it should be:

     b"idle\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"

Let's do that in a different PR. At any rate this code could have been auto-gen'd in so we might have to update it there too.

Copy link
Contributor

@pcrumley pcrumley left a comment

Choose a reason for hiding this comment

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

So because of the binary files, I can't really approve this code. I pulled it down locally and ran git diff on the files and it also just says the file changed.

I am guessing these are auto-gen'd and totally fine to accept but I think someone else who has more context about what can and can't change should review this. I don't feel comfortable approving it.

@silverjam @notoriaga See discussion above

@notoriaga
Copy link
Contributor

So because of the binary files, I can't really approve this code. I pulled it down locally and ran git diff on the files and it also just says the file changed.

I am guessing these are auto-gen'd and totally fine to accept but I think someone else who has more context about what can and can't change should review this. I don't feel comfortable approving it.

@silverjam @notoriaga See discussion above

This should get rid of the null bytes - #1293 still shows up as bin for me. Maybe takes a bit to update or has to be merged into main

@pcrumley
Copy link
Contributor

@notoriaga Thanks for fixing that null bytes issue so quickly, i know it was a bit nitpicky but i think it is better this way.

I think the reason it is showing it as a binary file is because the diff against main still shows the non-printable characters. I am not a git expert, but i think we could fix it with a .gitattributes file.

@adrian-kong adrian-kong merged commit 8d73e54 into master Jan 27, 2023
@adrian-kong adrian-kong deleted the adrian/fmt branch January 27, 2023 22:14
@pcrumley
Copy link
Contributor

see this pr which would render even the old files with the null bytes: #1294

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