Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix all lints currently disabled in scripts/sync.sh #565

Open
5 tasks
ia0 opened this issue Aug 5, 2024 · 5 comments
Open
5 tasks

Fix all lints currently disabled in scripts/sync.sh #565

ia0 opened this issue Aug 5, 2024 · 5 comments
Assignees
Labels
for:maintainability Improves maintainers life good first issue Good for newcomers needs:implementation Needs implementation to complete

Comments

@ia0
Copy link
Member

ia0 commented Aug 5, 2024

We want to enable the following lints:

  • rust.elided-lifetimes-in-paths
  • rust.missing-debug-implementations (not sure if good thing)
  • rust.missing-docs for all published libraries
  • rust.unreachable-pub
  • rust.unused-results
@ia0 ia0 added good first issue Good for newcomers needs:implementation Needs implementation to complete for:maintainability Improves maintainers life labels Aug 5, 2024
@devesh-2002
Copy link

May I take this issue?

@ia0
Copy link
Member Author

ia0 commented Aug 6, 2024

Of course, that would be great. That's a good first issue. You can start by changing those 2 lines:

wasefire/scripts/sync.sh

Lines 33 to 34 in 5da7a5e

# add_lint $file warn rust.elided-lifetimes-in-paths
# add_lint $file warn rust.missing-debug-implementations

into (I'm removing missing-debug-implementations because I'm not yet convinced it's a good thing to have):

  case $crate in
    prelude) add_lint $file warn rust.elided-lifetimes-in-paths ;;
  esac

Then run ./scripts/sync.sh (note that you might need to run ./scripts/setup.sh the very first time you clone the repository). This will add the lint to the prelude Cargo.toml. Then you can run ./test.sh from the crates/prelude directory and fix any warning.

Once there are no warnings, you can create your first PR.

Then, you can add more and more crates to the case pattern, like:

  case $crate in
    board|prelude) add_lint $file warn rust.elided-lifetimes-in-paths ;;
  esac

And create one PR for each non-trivial crate (you can group multiple crates together if there's nothing or almost nothing to change).

The final state (i.e. when adding the last remaning crate) should simply be:

  add_lint $file warn rust.elided-lifetimes-in-paths

Thanks! Feel free to ask questions if you're blocked.

@devesh-2002
Copy link

devesh-2002 commented Aug 6, 2024

Thank you @ia0 for the detailed instructions! I'll keep these in mind

@devesh-2002
Copy link

devesh-2002 commented Aug 9, 2024

@ia0 , Hello. I ran ./scripts/setup.sh and then ./scripts/sync.sh. But it gives me a duplicate key error. Would you please guide me in resolving this error?

 error: invalid table header
duplicate key `lints` in document root
  --> crates/xtask/Cargo.toml:26:1
   |
26 | [lints]
   | ^
   |

@ia0
Copy link
Member Author

ia0 commented Aug 9, 2024

To debug:

  • Could you push your working directory to a branch on your fork?
  • What is your operating system?
  • What is your version of sed? (sed --version)
  • What is your version of sh? (sh --version)
  • What is the path where wasefire is cloned?

I suspect that one of those 2 lines doesn't behave as expected in your case:

wasefire/scripts/sync.sh

Lines 30 to 31 in d32a3d9

sed -i '/^\[lints\]$/q' $file
[ "$(tail -n1 $file)" = '[lints]' ] || printf '\n[lints]\n' >> $file

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
for:maintainability Improves maintainers life good first issue Good for newcomers needs:implementation Needs implementation to complete
Projects
None yet
Development

No branches or pull requests

2 participants