Skip to content

Conversation

@LukaszMrugala
Copy link
Contributor

Ruff formatting applied to environment.py file.
Linting problems detected by ruff fixed.
As this majorly changes file lines, help strings were normalised as multiline strings.
Parameters mentioned in help strings now are inside quotation marks.
All quotation marks inside help strings changed to single quotes.
Minor typos and grammar errors fixed.

Unit test changed to incorporate changes.

We didn't want to make a have commit fixing all ruff formatting, as it would be too big and unwieldy. (As I understand the discussion in the PR introducing ruff here) Instead, we wanted a gradual and incremental approach, where new files would adhere to ruff and old ones would get there >eventually<.

However, coupling such big formatting changes with logic changes can make the differences and bugs hard to spot in the PR, so it would be better to bring up the older files in separate PRs. Otherwise, we may never have compliant Twister files.

Concerning Twister, twisterlib is my main concern. The environment.py was deemed a good starting point. After those, I'd like to tackle the Twister test files.

The matter of PR sizes is an open question that I'd like to introduce here. Should there be a PR per file? One PR per directory group (e.g. pylib or twisterlib in this case)? Every file touched by the formatter can often be majorly changed, so I'd assume that the number of additions and deletions is a good guide.

Ruff formatting applied to environment.py file.
Linting problems detected by ruff fixed.
As this majorly changes file lines, help strings
were normalised as multiline strings.
Parameters mentioned in help strings now are
inside quotation marks.
All quotation marks inside help strings
changed to single quotes.
Minor typos and grammar errors fixed.

Unit test changed to incorporate changes.

Signed-off-by: Lukasz Mrugala <[email protected]>
@pdgendt
Copy link
Contributor

pdgendt commented Nov 21, 2024

For traceability I would advice to focus on the linter fixes and leave the formatting for now. The linter fixes are currently hidden and might have side-effects that need to be looked at in a review.

@LukaszMrugala
Copy link
Contributor Author

For traceability I would advice to focus on the linter fixes and leave the formatting for now. The linter fixes are currently hidden and might have side-effects that need to be looked at in a review.

I'd wager that linter fixes could be bunched together, as the aren't as expansive as formatter-based changes. I didn't verify all files, but I might be able to bundle all twisterlib linter fixes in one PR.

@pdgendt
Copy link
Contributor

pdgendt commented Nov 21, 2024

I have done something similar for the west runners here #81667

@LukaszMrugala
Copy link
Contributor Author

I have done something similar for the west runners here #81667

I'll maybe keep this one as a draft, so as to first have the fixes merged (via a separate PR akin to yours for west runners), and then do reformatting.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 21, 2025
@github-actions github-actions bot closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants