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

Style check based on black #3827

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

clebergnu
Copy link
Contributor

Avocado has been using black to enforce a code style for a long time now, with very good results. Let's expand the same to Avocado-VT. With a common code style across Avocado and Avocado-VT it'll be easier to move libraries to the common autils repo.

Reference: avocado-framework/autils#3

@clebergnu clebergnu force-pushed the style_black branch 4 times, most recently from a6bdffd to a9fffa5 Compare January 15, 2024 13:55
@clebergnu clebergnu marked this pull request as ready for review January 15, 2024 13:58
@clebergnu clebergnu self-assigned this Jan 15, 2024
Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @clebergnu it LGTM, but I was able to review only the black enablement related changes, not the style changes itself (I presume they were made by black). Because of this, I think it is necessary to test these changes with some real avocado-vt test suite. @luckyh can you or any of avocado-vt maintainers test this with some bigger avocado-vt test suite?

@luckyh
Copy link
Contributor

luckyh commented Jan 25, 2024

@YongxueHong @vivianQizhu @chloerh @dzhengfy @chunfuwen adding you to this thread as well for your awareness, thanks.

@luckyh
Copy link
Contributor

luckyh commented Jan 25, 2024

@yanan-fu would you mind to help trigger a big test loop to verify this? thanks in advance!

@clebergnu
Copy link
Contributor Author

clebergnu commented Feb 14, 2024

@yanan-fu did you get a chance to test this? Thanks in advance!

@clebergnu
Copy link
Contributor Author

Hi @luckyh @yanan-fu, let me know if you are able to advance the review of this PR. Thanks!

@luckyh
Copy link
Contributor

luckyh commented Feb 29, 2024

let me know if you are able to advance the review of this PR. Thanks!

Hi @clebergnu , sorry for the late response (just came back from leave). Apart from needing to resolve the conflicts it LGTM. Thanks!

@yanan-fu
Copy link
Contributor

@yanan-fu did you get a chance to test this? Thanks in advance!

Hi @clebergnu , sorry for the late reply as Holiday and PTOs, as patch conflict, it can not be applied successfully.
Could you please resolve the conflict and i can start the test ASAP, thanks a lot!

@clebergnu
Copy link
Contributor Author

@yanan-fu did you get a chance to test this? Thanks in advance!

Hi @clebergnu , sorry for the late reply as Holiday and PTOs, as patch conflict, it can not be applied successfully. Could you please resolve the conflict and i can start the test ASAP, thanks a lot!

Hi @yanan-fu,

Conflicts have been resolved with a rebase. Please note that new conflicts will most certainly appear as other PRs are merged.

Thanks.

@yanan-fu
Copy link
Contributor

yanan-fu commented Mar 4, 2024

@yanan-fu did you get a chance to test this? Thanks in advance!

Hi @clebergnu , sorry for the late reply as Holiday and PTOs, as patch conflict, it can not be applied successfully. Could you please resolve the conflict and i can start the test ASAP, thanks a lot!

Hi @yanan-fu,

Conflicts have been resolved with a rebase. Please note that new conflicts will most certainly appear as other PRs are merged.

Thanks.

@clebergnu Regression test in on going, result will be commented soon.

@yanan-fu
Copy link
Contributor

yanan-fu commented Mar 5, 2024

Regression test all PASS.

LGTM, I think it is time to resolve the conflict and merge it now. Thanks all!

Avocado has been using black to enforce a code style for a long time
now, with very good results.  Let's expand the same to Avocado-VT.
With a common code style across Avocado and Avocado-VT it'll be easier
to move libraries to the common autils repo.

Reference: avocado-framework/autils#3
Signed-off-by: Cleber Rosa <[email protected]>
@clebergnu
Copy link
Contributor Author

Regression test all PASS.

LGTM, I think it is time to resolve the conflict and merge it now. Thanks all!

Thanks @yanan-fu. I'm considering your ACK on the regression testing a second positive review, and will expedite the merge of these changes to avoid further conflicts (once CI checks pass).

@clebergnu clebergnu merged commit 05bc5f7 into avocado-framework:master Mar 5, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants