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

ci: add script for linting & formatting code #116

Closed

Conversation

chungquantin
Copy link
Collaborator

@chungquantin chungquantin commented Jul 22, 2024

Link to issue: #115

  • Apply lint suggestion
  • Format check all packages
  • Apply the format (checkout branch)

@Daanvdplas Daanvdplas requested a review from peterwht July 22, 2024 21:28
@Daanvdplas
Copy link
Collaborator

Daanvdplas commented Jul 22, 2024

FYI: we have some formatting checks in place here. Here you propose CI to make formatting commits if it doesn't fulfil the requirements. In addition, the nightly formatting is used. The latter is something we were already talking about internally and could perhaps be a good change. Not sure about the auto formatting. What do you think @evilrobot-01?

I do like the approval for executing the CI!

@peterwht
Copy link
Collaborator

peterwht commented Jul 25, 2024

Thanks for doing this Tin!

  • I am okay with auto-applying cargo fmt (maybe not nightly version though)
  • I am slightly concerned with auto-applying clippy fixes as they can sometimes introduce unintended side-effects (rarely). I think it would be better to prevent merging until clippy has been resolved by the PR author.

@evilrobot-01
Copy link
Collaborator

Nothing against auto-formatting. Nightly allows for a few additional nice things it seems (e.g. grouping imports) if desired, and as shared elsewhere, appears to be in use by the Polkadot SDK: https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/STYLE_GUIDE.md.

Agree with Peter, probably best to manually address clippy warnings for now.

@evilrobot-01
Copy link
Collaborator

Instead of adding an additional workflow, can we rather rename build.yml to ci.yml and then add these changes into that workflow?

@evilrobot-01
Copy link
Collaborator

Also imagine that this might create some 'churn' for those that dont auto-format locally, whereby branch is pushed, CI formats and adds a commit and then local branch is out of sync. This can hopefully be eliminated by a proper local setup, which probably enforces formatting locally more so than currently. Either you sort it out to avoid any hassle, or GH sorts it for you and you have to cope with any hassle.

@Daanvdplas
Copy link
Collaborator

Also imagine that this might create some 'churn' for those that dont auto-format locally, whereby branch is pushed, CI formats and adds a commit and then local branch is out of sync. This can hopefully be eliminated by a proper local setup, which probably enforces formatting locally more so than currently. Either you sort it out to avoid any hassle, or GH sorts it for you and you have to cope with any hassle.

For this reason, is it then not more simple if we do a CI check on nightly and have the developer fix it themselves?

@chungquantin
Copy link
Collaborator Author

Hence, I think would be better to have:

  • Check clippy and prevent from merging until it is resolved
  • For formatting, we still want to apply it automatically to the PR code on pushed to keep the codebase style consistent

@chungquantin
Copy link
Collaborator Author

As there are a huge conflict in this PR, I will close it and create the another.

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