Lint refactoring

[ewels]

Hi all,

I couldn't help myself sorry and had a stab at doing some of the linting refactoring that we're discussing in #805. After writing that issue and planning out what needs doing it was rattling around my head with ideas (coding without a keyboard) so I thrashed out a bit of it during lunch.

Still WIP but I thought that I should open a PR so that we can work on this together. Feel free to change the base and merge to a new branch on the main repo if anyone wants to keep adding to the changes.

I haven't touched the tests and haven't started breaking up the tests, but I've done the config, the named tests (instead of numeric) and a bunch of other tidying. I also started looking into improving the sphinx docs so that we can use inline documentation instead of having to maintain the lint_errors.md file in parallel.

Phil

[ewels]

Love it 😆

[ewels]

ok, so rewriting the tests is a bit of a scarily large job I'm realising. But at the moment they are pretty crap so I think it's worthwhile.

I've made a bit of a start, it's tempting to just delete all of the tests and start fresh, but I think that some do have useful bits of code that may be worth keeping. Just needs some grind to go through them all.

[ewels]

On a happier note, the sphinx docs seem to be working pretty nicely now! I think that this is a better way to document the lint tests - should be a lot easier to keep the docs up to date this way.

Still need to port over the existing markdown documentation into the function docstrings. But that's just a copy + paste job.

[ewels]

Ok, I did a bit of a late-night docs marathon and have now ported all docs from docs/errors.md into the function docstrings.

I think that this means that this mammoth PR is now ready to merge to dev if everyone is happy. We will need to check how this is deployed on the website, but if it is on the dev branch then we will want to set up some automatic redirects right away from https://nf-co.re/errors to the new Sphinx lint test docs.

We still have a lot of python tests to write, but that can be done incrementally in smaller PRs I think, as it is not so essential to functionality so does not all need to be merged in one go.

[ewels]

Other website option: the Sphinx docs are compiled to static HTML so we could even try to be clever and pull that out and inject it into the nice nf-core styled website under whatever URL we want (eg. the existing /errors URL?).

[KevinMenden]

One thing I find odd is that you store the lint code away in __init__.py
Just wondering what's the incentive behind this (instead of having a lint.py file)?
(Apart from hiding the code from curious people stuck_out_tongue_closed_eyes )

This is because I put these files into a lint directory - you have to have a file called __init__.py to make that directory function as a Python module (even if that file is totally empty). I could have made a file called lint.py and imported that, but then it would be import nf_core.lint.lint which is kind of uglier.

In MultiQC I fudge my way around this in the modules by importing the file with the nicer filename into __init__.py, eg. here. Where __init__.py looks like this with an adjacent bamtools.py file:

from .bamtools import MultiqcModule

We could do this here, but I thought it was actually kind of nice having the core lint code in __init__.py because (a) it floats to the top of the directory when alphabetically sorted and (b) it makes it obviously separate from the rest of the files which contain lint tests of the same name.

But yeah, downside is that you have to know where to look for it wink

Sure - I'm aware of the advantages. I just think that for someone who hasn't written the code, it will be quite confusing to find this method. Here I would have gone for better readability and a bit more boilerplate code ;) But that's just my style-opinion so nevermind, doesn't affect functionality anyway

[KevinMenden]

Had another look at it, as I saw most of it already. Looks good I think :)

Only thing is that there are still some tests missing for lint function, e.g. for the README check.
But also for the schema lint, params and a couple of other tests. Guess it makes sense to include them in this PR already, right?

[ewels]

Yeah it would be nice but I'm not sure if I'll have time to write those tests this side of Christmas. That's why I did a bit of a sprint to finish porting over the docs last night.

I have a bunch of ideas for further improvements to the tests as well. Happy to leave this PR open as a WIP but was thinking that it risks getting behind the main repo then so maybe better to merge now and do incremental improvements onwards.

[apeltzer]

I'd rather open an issue with a list of missing tests and cover these then with smaller (and thus easier to review) PRs. That could also then be done by others than Phil , which simultaneously means we're not just reviewing but also testing (+ breaking ;-)) things :-)

[KevinMenden]

Okay - agree that this PR is already pretty large hand hard to follow.
Just wanted to mention the tests as reviewer duty :-)

[ewels]

Only thing that might be worth changing before the merge is the code location for the core lint code, instead of being in lint/__init__.py

@KevinMenden any suggestions for where you would have expected to find it?

[KevinMenden]

To me, the most natural way would be a lint.py script either in the nf_core directory or in the lint directory - although in the latter case it should probably named lint_main.py or something like this.
But as I said, this is really only relevant if something is new to the code and looking for it. So actually a small comment in the main script where the lint function was called would be totally sufficient (could a small word in a new PR).

It might also be just me 😁 So I would say let's merge this and if you agree add a small comment in another release - have to add some tests anyway.

[ewels]

Ok sounds good 👍🏻 +1 for a code comment, I think that keeps the best of both worlds..

[ewels]

ok good, it looks like https://nf-co.re/errors is still there..

[ewels]

The docs build failed though: https://github.com/nf-core/tools/runs/1550064554?check_suite_focus=true

[ewels]

Well, #813 fixed the action but now the entire API docs have gone 😆 https://nf-co.re/tools-docs/

[ewels]

Ok, and fixed the fix - docs now showing: https://nf-co.re/tools-docs/ 🚀

[KevinMenden]

😌 nice! :)