uclahs-cds / package-moPepGen

Multi-Omics Peptide Generator
https://uclahs-cds.github.io/package-moPepGen/
GNU General Public License v2.0
6 stars 1 forks source link

Fix The Usage Section in Docs #837

Closed zhuchcn closed 9 months ago

zhuchcn commented 9 months ago

Description

The usage section in docs has mkdocs instead of moPepGen (see below). Fixed it to show the correct command name.

image

Also fixed grammar issues in documentations.

Closes #...

Checklist

zhuchcn commented 9 months ago

@nwiltsie Hi Nick, help is needed! I just updated the cicd docker image from docker hub to ghcr. Didn't realize this until today. Why is my .pylintrc config file not recognized? I specifically disabled some warnings but they are still raised by pylint.

nwiltsie commented 9 months ago

Your .pylintrc file is being recognized - your changes in cec7b27 reduced the warnings from 28 to 12.

We updated the version of pylint when we migrated from the old container registry and I think there are a few new checks in there.

zhuchcn commented 9 months ago

Thanks Nick! I think when I clicked on the github action output link, it redirected me to an old run which has all the error messages that are supposed to be fixed. It's probably just GitHub being laggy. So now I only have the Cyclic import left. The old pylint doesn't complain about it. Should we just disable it, too? If there is a cyclic import, the problem won't run anyway, right?

nwiltsie commented 9 months ago

You can also test the CICD process locally with the bllint script (https://github.com/uclahs-cds/docker-CICD-base?tab=readme-ov-file#how-to-run-locally). In order for that to work you'll need to make a .cicd-env file (you can literally duplicate this PR, it had the same change).

nwiltsie commented 9 months ago

So now I only have the Cyclic import left. The old pylint doesn't complain about it. Should we just disable it, too? If there is a cyclic import, the problem won't run anyway, right?

I don't think it's a good idea to blindly disable, but fixing that code is out-of-scope for this PR. I'd recommend disabling it now but adding an issue (linking back here) to re-enable that check and fix the offending code.

zhuchcn commented 9 months ago

It's a lot of work to fix them all. But since the program is running, and the tests are all passed, would you say it's a relatively small issue for now?

Also, do you know why they were not caught by the old pylint?

nwiltsie commented 9 months ago

For the purposes of this PR, things are golden! I don't know enough about the code in question to prioritize the new issue relative to other work, so I don't want to call it a "small issue", but at the very least the code is in the same state it was yesterday.

The old pylint didn't catch these circular imports because we were previously linting each file individually, but now we are linting them all simultaneously. That's also why the duplicate code checker is triggering - it could never see both files at once before.

zhuchcn commented 9 months ago

I think I incorporated all your suggested changes. I also updated the repo name in all occurrences. @lydiayliu

lydiayliu commented 9 months ago

I don't want to go through this again XD just merge