Skip to content

Linter CI part 1 #52

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

Merged
merged 49 commits into from
Mar 31, 2025
Merged

Linter CI part 1 #52

merged 49 commits into from
Mar 31, 2025

Conversation

yaswant
Copy link
Contributor

@yaswant yaswant commented Jan 13, 2025

Description

Summary

Prepare changes for #72

Changes

Added

  • Update scripts to pass CI checks

Dependency

None.

Impact

None.

Issues addressed

Resolves

#51

Coordinated merge

#72

Checklist

  • I have performed a self-review of my own changes

@yaswant yaswant self-assigned this Jan 13, 2025
@yaswant yaswant linked an issue Jan 13, 2025 that may be closed by this pull request
Copy link
Contributor Author

@yaswant yaswant left a comment

Choose a reason for hiding this comment

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

some minor changes in ruff.toml

@yaswant
Copy link
Contributor Author

yaswant commented Jan 24, 2025

@yaswant
Copy link
Contributor Author

yaswant commented Mar 19, 2025

@james-bruten-mo frustration won, eventually 😉

Copy link
Contributor

@Pierre-siddall Pierre-siddall left a comment

Choose a reason for hiding this comment

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

This is all good stuff and seems like it will run smoothly, I've added a few suggestions regarding reverts of black changes and a comment about the future unification of the keywords list which I believe we've already picked up on.

@yaswant
Copy link
Contributor Author

yaswant commented Mar 20, 2025

@Pierre-siddall you can open a new issue/pr to address your suggestion. This PR has already got very crowded with multiple ideas which is a digression from the original purpose. Let's get the CI workflow in first.

@Pierre-siddall
Copy link
Contributor

Pierre-siddall commented Mar 20, 2025

@Pierre-siddall you can open a new issue/pr to address your suggestion. This PR has already got very crowded with multiple ideas which is a digression from the original purpose. Let's get the CI workflow in first.

@yaswant I'll probably add these suggestions into a new issue.

Copy link
Contributor

@Pierre-siddall Pierre-siddall left a comment

Choose a reason for hiding this comment

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

The previously suggested changes are better suited to a new issue or PR. Therefore as the semantics of this code looks fine I'm happy to approve this PR 😄.

james-bruten-mo and others added 2 commits March 20, 2025 11:45
Co-authored-by: Pierre Siddall <[email protected]>
Co-authored-by: Pierre Siddall <[email protected]>
@james-bruten-mo
Copy link
Contributor

Part of the intention of black is that it forces you to do the styling a single way so everything is consistent. But the last string in the suite_report suggestion is a good one to change as it won't combine strings that have already been split up so I've committed that.
As Yash suggests, the keywords work should be it's own ticket

@t00sa
Copy link
Contributor

t00sa commented Mar 24, 2025

This PR is much to large and untidy and needs to be split up. It should be fairly straight forward to separate the original workflow changes, and split off the various script changes into their own items. This shouldn't greatly increase the workload if individual components have already been reviewed.

@james-bruten-mo
Copy link
Contributor

I've just deleted the .github directory so this is now only the styling changes. Will open a new PR for the CI in a sec...

@james-bruten-mo james-bruten-mo mentioned this pull request Mar 24, 2025
2 tasks
@yaswant
Copy link
Contributor Author

yaswant commented Mar 24, 2025

@james-bruten-mo I was going to suggest other way around:, but since you have already done this, I suggest you bring back README changes to this branch

@james-bruten-mo
Copy link
Contributor

@james-bruten-mo I was going to suggest other way around:, but since you have already done this, I suggest you bring back README changes to this branch

Yes, I was thinking that way, but the .github changes were a lot more self-contained so easier to move!
I'll bring back the readme though

@yaswant yaswant changed the title Linter CI part 1 Linter CI part 2 Mar 24, 2025
@yaswant yaswant changed the title Linter CI part 2 Linter CI part 1 Mar 24, 2025
Copy link
Contributor

@t00sa t00sa left a comment

Choose a reason for hiding this comment

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

Almost all of this is fine. A couple of minor fixes for places were black hasn't concatenated strings together properly.

james-bruten-mo and others added 2 commits March 24, 2025 13:15
Copy link
Contributor

@t00sa t00sa left a comment

Choose a reason for hiding this comment

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

Thanks. I'm happy to approve this

@Pierre-siddall Pierre-siddall merged commit 70d6198 into main Mar 31, 2025
3 checks passed
@Pierre-siddall Pierre-siddall deleted the feature/ci_linter branch March 31, 2025 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include code linting workflow
4 participants