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

Linter updates #684

Merged
merged 74 commits into from
Dec 7, 2023
Merged

Linter updates #684

merged 74 commits into from
Dec 7, 2023

Conversation

GernotMaier
Copy link
Contributor

@GernotMaier GernotMaier commented Nov 20, 2023

This add linters to the CI with the goal of linting every single file type in simtools:

  • python (flake8)
  • python (black)
  • python (isort)
  • python (pylint)
  • docker
  • github action
  • rst (remove this, as no good solution was found)
  • yaml
  • markdown
  • bash
  • pyproject.toml
  • CITATION.cff
  • check for non-ascii characters
  • natural language
  • .env files
  • copy and paste duplications

Most of it is achieved by using the super-lint github action.

Missing files not covered by linters:

  • ecsv
  • LICENSE
  • Makefile
  • zst
  • corsikaio

For pylint, I've removed the 500 lines of default configuration which we had in pyproject.toml. I think we should list only the non-default values (otherwise readability will be hard).

Note that the extensive linting results in a run time of 10-12 min of this workflow.

@GernotMaier GernotMaier self-assigned this Nov 20, 2023
@GernotMaier GernotMaier linked an issue Nov 20, 2023 that may be closed by this pull request
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (9e9534f) 82.16% compared to head (a5bd97a) 82.18%.
Report is 2 commits behind head on main.

Files Patch % Lines
simtools/db_handler.py 50.00% 3 Missing ⚠️
simtools/psf_analysis.py 78.57% 3 Missing ⚠️
simtools/simtel/simtel_histograms.py 40.00% 3 Missing ⚠️
simtools/simtel/simtel_runner.py 60.00% 2 Missing ⚠️
simtools/corsika/corsika_histograms_visualize.py 96.42% 1 Missing ⚠️
simtools/simtel/simtel_runner_array.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #684      +/-   ##
==========================================
+ Coverage   82.16%   82.18%   +0.01%     
==========================================
  Files          42       42              
  Lines        6191     6196       +5     
==========================================
+ Hits         5087     5092       +5     
  Misses       1104     1104              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@orelgueta
Copy link
Contributor

I "fixed" all of the issues reported by the CI, with the exception of W0632 (which I do not really understand). I put "fixed" in quotations because there were a few places where I just added a disable comment to pylint. I will explain those individually as comments on the code.

@GernotMaier
Copy link
Contributor Author

I've fixed the remaining one (the numpy.unravel_index can return a tuble of any length, but we definitely expect only two values here).

Merged also #714 into main / this branch to solve the corsika_parameter_file complain.

Pylint is running through without any complaints now, very good!!

(look for the super-linter complain now)

@GernotMaier
Copy link
Contributor Author

The linter works, see e.g., https://github.com/gammasim/simtools/actions/runs/7125191095/job/19400677771
(sorting of issues should be covered by the pre-commit hook, but these are all commits from myself at a day where my pre-commit hood was broken).

@GernotMaier
Copy link
Contributor Author

@orelgueta , @VictorBarbosaMartins - this pull request is now ready for final review. Please have a look and let me know when it is ready to merge.

Have a look at the 'checks' panel, super-linter nicely lists all the linters.

Lot's of trivial changes, but some important ones. Very nice.

@VictorBarbosaMartins
Copy link
Contributor

https://github.com/gammasim/simtools/actions/runs/7125191095/job/19400677771

In the output there are some errors, but I think they were corrected by the linter?

2023-12-07 07:47:24 [ERROR]   ERRORS FOUND in PYTHON_ISORT:[3]
2023-12-07 07:47:25 [FATAL]   Exiting with errors found!

@GernotMaier
Copy link
Contributor Author

Yes - this is the example to show you that it works and that the linter picks up errors.

@orelgueta
Copy link
Contributor

I reviewed this issue yesterday and do not have any complaints. So from my point of view, it can be merged.

@VictorBarbosaMartins
Copy link
Contributor

I was wondering if there is a way to correct the line breaks of the doc string as well. I see line breaks in some places that are way below the limit. And sometimes line are broken with "" in the docstring, but most of the time not. (not too important I guess)

@GernotMaier
Copy link
Contributor Author

Examples?

Copy link
Contributor

@VictorBarbosaMartins VictorBarbosaMartins left a comment

Choose a reason for hiding this comment

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

Ok, I am approving with some more comments to consider. Thank you for these changes, it will facilitate a lot our work and maintain the quality of the code.

pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
simtools/corsika/corsika_histograms.py Outdated Show resolved Hide resolved
simtools/data_model/model_data_writer.py Show resolved Hide resolved
@VictorBarbosaMartins
Copy link
Contributor

Examples?

Too early break:

Load photon list from sim_telarray file and compute centroids, psf containers, effective area,
as well as plot the image as a 2D histogram.
Internal units: photon positions in cm internally.

@VictorBarbosaMartins
Copy link
Contributor

Examples?

Breaking with a slash:

Camera class, defining pixel layout including rotation, finding neighbour pixels, calculating\
FoV and plotting the camera.
Parameters
----------
telescope_model_name: string
As provided by the telescope model method TelescopeModel (ex South-LST-1).
camera_config_file: string
The sim_telarray file name.
focal_length: float
The focal length of the camera in (preferably the effective focal length), assumed to be \

@GernotMaier
Copy link
Contributor Author

I don't think we have a strict rule with the line breaks, but I don't think we need to fix it. Both ways are probably fine.

Signed-off-by: Victor Barbosa Martins <[email protected]>
@VictorBarbosaMartins
Copy link
Contributor

Thanks! I think we can merge after the tests pass!

@GernotMaier GernotMaier merged commit 92b1c92 into main Dec 7, 2023
9 checks passed
@GernotMaier GernotMaier deleted the linter branch December 7, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2023_maintenance Issues to be fixed in 2023 maintenance
Projects
None yet
3 participants