Skip to content

Ci linting action#924

Open
psg-19 wants to merge 1 commit into
getfloresta:masterfrom
psg-19:ci-linting-action
Open

Ci linting action#924
psg-19 wants to merge 1 commit into
getfloresta:masterfrom
psg-19:ci-linting-action

Conversation

@psg-19
Copy link
Copy Markdown
Contributor

@psg-19 psg-19 commented Apr 2, 2026

Description and Notes

Closes #677
Currently, typos runs inside the Rust CI linting job in rust.yml, even though it's language agnostic and checks the entire project not just rust code. Similarly, Python linting is duplicated in functional.yml.

This PR creates a dedicated lint.yml workflow that separates language agnostic linting from rust specific check.

How to verify the changes you have done?

the ci passes in my fork https://github.com/psg-19/Floresta/actions

Contributor Checklist

  • I've followed the contribution guidelines
  • I've verified one of the following:
    • Ran just pcc (recommended but slower)
    • Ran just lint-features '-- -D warnings' && cargo test --release
    • Confirmed CI passed on my fork
  • I've linked any related issue(s) in the sections above

@moisesPompilio
Copy link
Copy Markdown
Collaborator

I think there are still some issues to be addressed in the issue for this PR to be considered as fully resolving it.

@psg-19
Copy link
Copy Markdown
Contributor Author

psg-19 commented Apr 5, 2026

I think there are still some issues to be addressed in the issue for this PR to be considered as fully resolving it.

Okay, I'll be reading the discussions actively and change the code accordingly.

@moisesPompilio
Copy link
Copy Markdown
Collaborator

Please leave this PR as a draft for now

Comment thread .github/workflows/rust.yml Outdated

jobs:
linting:
rust-lint:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding of the issue is that this should also be under lint.yml, right @jaoleal?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

renaming the rust job id from linting to rust-lint may break required status checks?
unless branch protection is updated?? am i right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My understanding of the issue is that this should also be under lint.yml, right @jaoleal?

So is it like this, we should move all linting stuff like typos, clippy, cargo fmt, cargo doc, Python lint to lint.yml and rust.yml only for testing and building

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding of the issue is that this should also be under lint.yml, right @jaoleal?

Yes, Ideally a centralized job for linting

@psg-19 psg-19 marked this pull request as draft April 7, 2026 07:33
@csgui csgui added the CI A change or issue related to CI label Apr 7, 2026
@csgui csgui added this to Floresta Apr 7, 2026
@github-project-automation github-project-automation Bot moved this to Backlog in Floresta Apr 7, 2026
Copy link
Copy Markdown
Member

@jaoleal jaoleal left a comment

Choose a reason for hiding this comment

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

Almost it, move the rust linting to lint.yml and could you also address #730 ? (Never mind, The diff would be huge and i prefer for someone of the team to address that.)

Comment thread .github/workflows/rust.yml Outdated

jobs:
linting:
rust-lint:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding of the issue is that this should also be under lint.yml, right @jaoleal?

Yes, Ideally a centralized job for linting

@psg-19
Copy link
Copy Markdown
Contributor Author

psg-19 commented Apr 14, 2026

Almost it, move the rust linting to lint.yml and could you also address #730 ? (Never mind, The diff would be huge and i prefer for someone of the team to address that.)

Done, moved rust linting to the new lint.yml file

@csgui
Copy link
Copy Markdown
Collaborator

csgui commented Apr 14, 2026

This PR is ready for review or is it a draft?

@psg-19 psg-19 marked this pull request as ready for review April 14, 2026 18:19
@jaoleal
Copy link
Copy Markdown
Member

jaoleal commented Apr 14, 2026

HI, @psg-19 appears to be almost there, only thing missing is the commit. I think this can be merged in a single commit,

For reference: https://github.com/getfloresta/Floresta/blob/master/CONTRIBUTING.md#commits

@psg-19
Copy link
Copy Markdown
Contributor Author

psg-19 commented Apr 14, 2026

HI, @psg-19 appears to be almost there, only thing missing is the commit. I think this can be merged in a single commit,

For reference: https://github.com/getfloresta/Floresta/blob/master/CONTRIBUTING.md#commits

Sure, will squash the commits to one commit and rebase

@psg-19 psg-19 force-pushed the ci-linting-action branch 3 times, most recently from 36cbeae to e1b7432 Compare April 14, 2026 18:35
@jaoleal
Copy link
Copy Markdown
Member

jaoleal commented Apr 14, 2026

e1b7432 is almost there, just remind that the reference ive sent to you also included instructions about how to write commit messages, as it is the commit will not pass CI.

@psg-19 psg-19 force-pushed the ci-linting-action branch 2 times, most recently from cb6d109 to b70aa98 Compare April 14, 2026 18:46
@psg-19
Copy link
Copy Markdown
Contributor Author

psg-19 commented Apr 15, 2026

e1b7432 is almost there, just remind that the reference ive sent to you also included instructions about how to write commit messages, as it is the commit will not pass CI.

Yeah, missed to fix the commit message. Did that now.

@jaoleal
Copy link
Copy Markdown
Member

jaoleal commented Apr 15, 2026

For the commit message, prefer

"ci: centralize lint jobs on lint.yml"

Also, would be nice for it to explain a little about the job, but the tittle is meaningfull enough

Copy link
Copy Markdown
Member

@jaoleal jaoleal left a comment

Choose a reason for hiding this comment

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

ACK de5c851

Copy link
Copy Markdown

@Vedd-Patel Vedd-Patel left a comment

Choose a reason for hiding this comment

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

ACK de5c851
only merge conflicts should be resolved
but one optional thing i've thought of is to add paths filters so heavy workflows don’t run on unrelated file changes

@csgui csgui moved this from Backlog to Needs review in Floresta May 8, 2026
@csgui csgui added this to the Q2/2026 milestone May 8, 2026
@jaoleal
Copy link
Copy Markdown
Member

jaoleal commented May 8, 2026

but one optional thing i've thought of is to add paths filters so heavy workflows don’t run on unrelated file changes

We can work on this on #960

@moisesPompilio
Copy link
Copy Markdown
Collaborator

Need rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI A change or issue related to CI

Projects

Status: Needs review

Development

Successfully merging this pull request may close these issues.

[Enhancement] Make a CI action only for linting

6 participants