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

Reformat all python script with Black #2099

Merged
merged 6 commits into from
Nov 1, 2023
Merged

Reformat all python script with Black #2099

merged 6 commits into from
Nov 1, 2023

Conversation

iarspider
Copy link
Contributor

No description provided.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @iarspider for branch master.

@iarspider, @cmsbuild, @smuzaffar, @aandvalenzuela can you please review it and eventually sign? Thanks.
@antoniovilela, @rappoccio, @sextonkennedy you are the release manager for this.
cms-bot commands are listed here

@smuzaffar
Copy link
Contributor

please test

@smuzaffar
Copy link
Contributor

test parameters:

  • full_cmssw = true

@smuzaffar
Copy link
Contributor

please test with cms-sw/cmsdist#8791

@smuzaffar
Copy link
Contributor

please test for CMSSW_8_0_X

@iarspider
Copy link
Contributor Author

iarspider commented Oct 31, 2023

Notice:

  1. I propose to make the resulting commit authored by cms-bot.

  2. There is also a way to make github (automatically) and git client (semi-automatically, unfortunately) to ignore the formatting commit - see, e.g., this article.

  3. I also propose to rename all python files to end with .py, and make original names symlinks (or dummy shell scripts running python3 <myname>.py). This will help to avoid explicitly listing all additional python scripts that need to be checked at push.

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c1879c/35524/summary.html
COMMIT: d64ce05
CMSSW: CMSSW_8_0_X_2023-10-29-0000/slc6_amd64_gcc530
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cms-bot/2099/35524/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 25 errors in the following unit tests:

---> test testNamedReference had ERRORS
---> test testORA_9 had ERRORS
---> test testNestedArrays had ERRORS
and more ...

Comparison Summary

Summary:

  • You potentially removed 5 lines from the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 15
  • DQMHistoTests: Total histograms compared: 1044635
  • DQMHistoTests: Total failures: 847
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1043680
  • DQMHistoTests: Total skipped: 108
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -14 KiB( 14 files compared)
  • Checked 62 log files, 40 edm output root files, 15 DQM output files

@iarspider
Copy link
Contributor Author

I found 25 errors in the following unit tests:

Also present in IBs

@smuzaffar
Copy link
Contributor

thanks @iarspider , changes looks good. Can we run black during the github actions?

1. I propose to make the resulting commit authored by cms-bot.

there is no need for cms-bot/build to be the author.

2. There is also a way to make github (automatically) and git client (semi-automatically, unfortunately) to ignore the formatting commit - see, e.g., [this article](https://moxio.com/blog/ignoring-bulk-change-commits-with-git-blame/).

If I am reading the above artica correctly then looks like git client (version >=2.32) can ignore a commits for blame but github and gitlab do not support it yet.

If you want then please go ahead and add .git-blame-ignore-revs file too

3. I also propose to rename all python files to end with `.py`, and make original names symlinks (or dummy shell scripts running `python3 <myname>.py`). This will help to avoid explicitly listing all additional python scripts that need to be checked at push.

sounds good. May be you can do this once this PR is merged.

@iarspider
Copy link
Contributor Author

iarspider commented Oct 31, 2023

Let's merge #2096 first, I will then rebase this one on top so that I don't have to reformat files touched in that one again.

@iarspider iarspider marked this pull request as ready for review October 31, 2023 17:01
@cmsbuild
Copy link
Contributor

Pull request #2099 was updated.

@cmsbuild
Copy link
Contributor

Pull request #2099 was updated.

@cmsbuild
Copy link
Contributor

Pull request #2099 was updated.

@cmsbuild
Copy link
Contributor

Pull request #2099 was updated.

@cmsbuild
Copy link
Contributor

Pull request #2099 was updated.

@smuzaffar
Copy link
Contributor

looks good, lets get this in

@smuzaffar smuzaffar merged commit 3d9b787 into master Nov 1, 2023
4 of 5 checks passed
@iarspider iarspider deleted the black branch November 1, 2023 10:46
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.

3 participants