Open ArtPoon opened 7 months ago
name: Pylint check
on: pull_request: branches:
jobs: pylint-check: runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v2
- name: Set up Python
uses: actions/setup-python@v2
with:
python-version: '3.10'
- name: Create virtual environment
run: python -m venv venv
working-directory: ${{ github.workspace }}
- name: Activate virtual environment and install dependencies
run: |
source venv/bin/activate
python -m pip install --upgrade pip
deactivate
working-directory: ${{ github.workspace }}
- name: Analyze code with Pylint
run: |
source venv/bin/activate
pylint $(git ls-files '*.py')
deactivate
working-directory: ${{ github.workspace }}
I've attached the list of warnings that pylint is currently throwing. We should resolve these before setting this Github Action or else our pull requests will fail
[pylint_report.log](https://github.com/PoonLab/covizu/files/14496057/pylint_report.log)
@WilliamZekaiWang can you please split a new branch off dev
and start working through the above log?
python3 -m venv venv # install virtual environment
source venv/bin/activate # start environment
pip install pylint # run this first time to install PyLint
pylint $(git ls-files '*.py')
deactivate # exit environment
@WilliamZekaiWang you do not need to address warnings for the deprecated files in covizu/deprecated
. We should be able to ignore that directory when running pylint
I've used autopep8
for a lot of the files, it seems pretty good for the most part. The way autopep8 works is that you can control how aggressive it is with its changes by adding --aggressive
however many times you want. I found that with no aggressives, it only removes white space at the end of lines, but as you add 2, it starts to introduce line breaks for lines exceeding > 100 characters in length (extra aggressives seemed to do nothing). I used 2 --aggressive
for the files and it seemed to keep most things working!
There are a few things that I realized autopep8
can't change which I'm still manually editing:
Sounds like a lot, but it does genuinely lighten the workload
I also used flynt
to auto change .format to f-strings. It doesn't work too well, but it can get some of them.
Thanks @WilliamZekaiWang for the update!
finishing up now, I have finished all the files in the main directory and the covizu directory. They all seemed to pass the unittests as well
generally, there's one small thing that I'm iffy on changing:
def retrieve_genomes(
by_lineage_in,
known_seqs,
ref_file,
outgroup=None,
earliest=True,
callback=None):
For this function in timetree.py
, pylint doesn't like it when there are greater than 5 arguments. However, I'm not sure how to change functions in these scenarios. One thought I had is that I change a few of these arguments to be a dictionary, such that when we call it:
retrieve_genomes(
by_lineage_in,
known_seqs,
ref_file,
{outgroup: None,
earliest: True,
callback: None})
also, pylint requires a docstring at the top of the file to explain what it does. I'm not quite sure what to add for most of the files
file specifically, there were 2 things I couldn't quite figure out. The first I can't find that error existing. The second error, I assumed value error?
batch.py:299:8: E0611: No name 'DuplicateDatabase' in module 'psycopg2.errors' (no-name-in-module)
line 299: from psycopg2.errors import DuplicateDatabase
beadplot.py:253:15: W0718: Catching too general exception BaseException (broad-exception-caught)
try:
ctree = Phylo.read(args.tree, 'newick')
except ValueError as e:
I have finished pep8-ing all files including utils and unittests
I think it's ok if we don't have docstrings for unittests. There are also a few TODO/FIXMEs in the files which raises a concern on pylint
specific functions/classes I don't really understand that need docstring:
pangolin
class in pangolin_utils.py
Another except that I'm not sure the specific catch would be: https://github.com/PoonLab/covizu/blob/ec04d1c2e016b4ecfe07791410fbd429e41bd5ce/covizu/utils/batch_utils.py#L251-L290
Functions that have greater than 6 arguments:
filter_problematic
in gisaid_utils.py
make_beadplots
in batch_utils.py
Callback's __init__
in progress_utils.py
, Functions that have too many local variables (>15) that I couldn't really figure out a way to break down:
make_beadplots
in batch_utils.py
parse_mutations
in seq_utils.py
stream_local
in local.py
. make_beadplots
also has too many branches
I should also add I tried my best to solve the >15 local variables by adding more function. I think most of them help with readability except parse_nexus
in timetree.py
, which I think I made more messy
2 classes had too few public methods:
SC2locator
in seq_utils.py
Callback
in progress_utils.py
make_beadplots
and other functions that have too many local variables or branchesI forgot that we are in the middle of refactoring make_beadplots.py
anyways in another branch, so pylint isn't wrong, the function is hideous (#497)
Let's wait until these changes are merged into dev
Bio.Phylo.BaseTree
object, I use the BaseTree
or Clade
constructor and manually build a small test fixture to compare against the function outputscripts/mut_annotations_edit.py
, a constellation is a combination of mutations that is characteristic of a specific SARS-CoV-2 lineageYou might find examples of unit tests for tree objects in https://github.com/PoonLab/covizu/blob/ca3379dc823bda5b6e849820a861d60047699d84/tests/test_beadplot.py#L8
Wait for PR #529 to be resolved before making a new PR from this branch into dev
@WilliamZekaiWang to create PR
Next step will be to issue a PR from dev
to master
, for which we will want to have set up GitHub actions for screening PEP8 compliance
Add as a GitHub action