search

icatcherplus / icatcher_plus

iCatcher+: Robust and automated annotation of infant gaze from videos collected in laboratory, field, and online studies
GNU General Public License v3.0
17 stars 19 forks source link

Ks/linting #45

Closed shehadak closed 1 year ago

shehadak commented 1 year ago

Added two Github Actions workflows for linting (Pylint) and formatting (Black). Pylint evaluates the quality of the pushed code, and Black reformats the code files to improve readability by future developers without changing its functionality.

This PR contains the added workflows in .github/workflows and a reformatting of the Python scripts in src/ and test/ using the Black auto-formating workflow.

yoterel commented 1 year ago

Thank you! Can you please submit this without any actual changes to the code (formatting, etc)? just the new workflows by themselves?, this will allow me to choose when to merge formatting independently of this upgrade, and save the headache of merging with the many changes coming from Quest soon.

yoterel commented 1 year ago

Q1: the result from pylint is a binary yes no? or a report with coverage?

In any case it should be added to the .toml file under dev, which will install it automatically if you change pip install . to pip install .[dev] like the test.yml workflow.

Q2: Should this really be a separate workflow?

shehadak commented 1 year ago

Thank you! Can you please submit this without any actual changes to the code (formatting, etc)? just the new workflows by themselves?, this will allow me to choose when to merge formatting independently of this upgrade, and save the headache of merging with the many changes coming from Quest soon.

That's a great thought, Yotam. I think the idea here is that code in every future PR will automatically be formatted with a new commit (the Auto-format code with black commit was done and pushed automatically by the formatter workflow). I understand your concern for wanting to keep this PR only for the workflows but keep in mind that the next PR will contain the formatting changes nonetheless. What do you think?

shehadak commented 1 year ago

Q1: the result from pylint is a binary yes no? or a report with coverage?

I believe detailed information about the static analysis is shown in the GitHub Actions logs (e.g. the logs for this PR's linting flow under "Analyzing code with pylint"). We can get a summary "report" by adding the report flag to Pylint, which I'm happy to do.

In any case it should be added to the .toml file under dev, which will install it automatically if you change pip install . to pip install .[dev] like the test.yml workflow.

Yup, I agree with you. Will do this.

Q2: Should this really be a separate workflow?

Hmm, I think that's a good point, but I believe it might be better to keep them separate. Keeping them as separate workflows provides a clear distinction between linting and formatting as they serve different purposes. Linting focuses on detecting code errors and standards violations, not just formatting issues, but formatting enforces a consistent style for readability and actually makes changes to the repository (commits to PRs with code reformatting). Also, by maintaining isolation, I think we get a clearer picture when one of the two processes fails, making quality checks more modular. What do you think?

yoterel commented 1 year ago

Closed for now until linting is required (black formatter was submitted separately over a new PR)