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

Workflow: Update Python and Actions + Black formatting Python files #756

Merged
merged 3 commits into from
Jan 15, 2024

Conversation

jdsika
Copy link
Contributor

@jdsika jdsika commented Jan 9, 2024

  • Add .venv/ and venv/ to .gitignore
  • Bump Python minimum to 3.8 as out of support
  • Update GitHub Marketplace Actions
  • Add workflow to check black formatting
  • Add requirements_develop.txt
  • Apply black formatting

@jdsika jdsika added the Quality Quality improvements. label Jan 9, 2024
@jdsika jdsika requested a review from pmai January 9, 2024 14:40
@jdsika jdsika added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jan 9, 2024
@jdsika jdsika changed the title Workflow: Update Python and Actions Workflow: Update Python and Actions + Black formatting Python files Jan 10, 2024
Copy link
Contributor

@PhRosenberger PhRosenberger left a comment

Choose a reason for hiding this comment

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

I am fine with the code refactoring and black formatting.
I cannot help that much with the workflow changes.
Hope to get some more detailed feedback from @pmai there.

@pmai
Copy link
Contributor

pmai commented Jan 12, 2024

I have created a separate PR for non-python related build workflow changes, that also adds support for newer protobuf releases (upto current 25.x releases), leaving this PR for python-related changes to workflow and python code changes, if wanted.

The apt-get update dance should be dropped, since we should never depend on anything more specific from the base system.

I'm not a huge fan of the black reformatting (or usually any automatic reformattings), as it just seems to change one set of idiosynchracies for another, but whatever floats anyones boat (I think the code should be rewritten in places to make it clearer, something the reformatting really doesn't address).

@pmai pmai force-pushed the update/workflow-dependencies branch from 4c85668 to 4f68172 Compare January 12, 2024 13:15
@pmai
Copy link
Contributor

pmai commented Jan 12, 2024

I've refactored to built upon the other workflow enhancements, so once those are merged only the python-related changes will remain in this PR.

@jdsika
Copy link
Contributor Author

jdsika commented Jan 15, 2024

I have created a separate PR for non-python related build workflow changes, that also adds support for newer protobuf releases (upto current 25.x releases), leaving this PR for python-related changes to workflow and python code changes, if wanted.

The apt-get update dance should be dropped, since we should never depend on anything more specific from the base system.

I'm not a huge fan of the black reformatting (or usually any automatic reformattings), as it just seems to change one set of idiosynchracies for another, but whatever floats anyones boat (I think the code should be rewritten in places to make it clearer, something the reformatting really doesn't address).

not that I disagree with your stance towards formatting but in our case this will be done the same way as in the osi-validation and if a lot of people work on something it is helping to provide a consistent visual.

@jdsika jdsika added this to the V3.7.0 milestone Jan 15, 2024
jdsika and others added 3 commits January 15, 2024 15:43
Signed-off-by: Carlo van Driesten <carlo.van-driesten@bmw.de>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai pmai force-pushed the update/workflow-dependencies branch from 4f68172 to 493bd51 Compare January 15, 2024 14:43
@pmai
Copy link
Contributor

pmai commented Jan 15, 2024

CCB 2014-01-15: Merge as-is.

@pmai pmai merged commit ec82b7c into master Jan 15, 2024
6 checks passed
@pmai pmai added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Apr 4, 2024
@jdsika
Copy link
Contributor Author

jdsika commented Apr 23, 2024

Reviewed for v3.7.0

@jdsika jdsika deleted the update/workflow-dependencies branch April 23, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Quality Quality improvements. ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants