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

Pin the nightly version to latest #10905

Closed
wants to merge 7 commits into from

Conversation

Abhishekkochar
Copy link
Contributor

Follow up on PR #10893

@@ -1,3 +1,4 @@
#![allow(missing_docs)]
Copy link
Member

Choose a reason for hiding this comment

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

can we change this to #![expect(missing_docs)] so we get a failure when we need to remove these? (ie its fixed upstream)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can. Although, this will trigger a warning.

Copy link
Member

Choose a reason for hiding this comment

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

what warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-09-16 at 12 49 59 PM

This behaviour is on by default.

.github/workflows/lint.yml Outdated Show resolved Hide resolved
mattsse
mattsse previously approved these changes Sep 14, 2024
@mattsse mattsse dismissed their stale review September 14, 2024 08:26

this isn't quite complete yet

@mattsse
Copy link
Collaborator

mattsse commented Sep 14, 2024

we still have a ton of clippy things to fix

…mentation warnings

Signed-off-by: Abhishekkochar <abhishekkochar2@gmail.com>
@Abhishekkochar
Copy link
Contributor Author

we still have a ton of clippy things to fix

Running this locally doesn't reproduce these errors. I have added these all needed tests expect test_utils. If clippy action fails because of this. I will add to them accordingly.

Signed-off-by: Abhishekkochar <abhishekkochar2@gmail.com>
@Abhishekkochar
Copy link
Contributor Author

@mattsse, there are around 300 instance of #[cfg(test)] that are need to be changed. Do you prefer to have these sorted in a single PR or multiple? Thanks

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

think it may be easier to separate lint for test target and lib target as two separate jobs in CI

Copy link
Member

Choose a reason for hiding this comment

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

could you revert everything but this file to main? then we can merge this. other conflicts have been fixed in smaller prs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be easier to open a new PR given several commits and conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

ye, probably right, this lint issue was a big pain for everyone, so had to push it through asap in many prs with smaller scope (easier to review/merge conflicts don't accumulate), thanks nonetheless

@emhane
Copy link
Member

emhane commented Sep 20, 2024

closing in favour of #11062

@emhane emhane closed this Sep 20, 2024
@Abhishekkochar Abhishekkochar deleted the ak/clippy-ci branch September 20, 2024 02:27
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