snakefmt icon indicating copy to clipboard operation
snakefmt copied to clipboard

Q: Can 2-line spacing around `configfile:` be switched off? Too much whitespace

Open corneliusroemer opened this issue 2 years ago • 6 comments

I just applied snakefmt to nextstrain/monkeypox. While I love the idea of a snakemake formatter and you've done a great job, the result for our main Snakefile is a bit too whitespacy.

In particular, it's ridiculous that there is whitesapce between if not config and configfile:

And two lines between build_dir = "results" and auspice_dir = "auspice is too much for my taste.

Is there a way to configure this? I couldn't figure out what all the config options are from the docs - all I could find were the CLI options, is there more?

from packaging import version
from augur.__version__ import __version__ as augur_version
import sys

min_version = "16.0.0"
if version.parse(augur_version) < version.parse(min_version):
    print("This pipeline needs a newer version of augur than you currently have...")
    print(f"Current augur version: {augur_version}. Minimum required: {min_version}")
    sys.exit(1)

if not config:

    configfile: "config/config_hmpxv1.yaml"


build_dir = "results"


auspice_dir = "auspice"

corneliusroemer avatar Jun 28 '22 18:06 corneliusroemer

Also consider this section. Too many line breaks :/

if config.get("data_source", None) == "lapis":

    include: "workflow/snakemake_rules/download_via_lapis.smk"


else:

    include: "workflow/snakemake_rules/prepare.smk"


include: "workflow/snakemake_rules/chores.smk"


include: "workflow/snakemake_rules/core.smk"


rule clean:

If/else should not be separated at all...

Better:

if config.get("data_source", None) == "lapis":
    include: "workflow/snakemake_rules/download_via_lapis.smk"
else:
    include: "workflow/snakemake_rules/prepare.smk"

include: "workflow/snakemake_rules/chores.smk"

include: "workflow/snakemake_rules/core.smk"


rule clean:

corneliusroemer avatar Jun 28 '22 18:06 corneliusroemer

Hi @corneliusroemer

The two spaces around configfile is a conscious decision we made along with @johanneskoester.

Regarding configuration, the only real configuration options are those in the CLI options, or those offered by black.

What do those example files you sent look like before formatting?

I appreciate it is normal to be able to configure tools, but the premise of black, and thus snakefmt, is that they're uncompromising/opinionated. They take the hassle out of thinking about how things should be formatted.

mbhall88 avatar Jun 28 '22 19:06 mbhall88

HI @mbhall88, thanks for getting back so quickly! Happy to show you the before/after. Sorry forgot about that here.

Before (better)

if not config:
    configfile: "config/config_hmpxv1.yaml"

build_dir = "results"

auspice_dir = "auspice"


rule all:

After:

if not config:

    configfile: "config/config_hmpxv1.yaml"


build_dir = "results"


auspice_dir = "auspice"


rule all:

corneliusroemer avatar Jun 28 '22 19:06 corneliusroemer

Hmm, the extra space added between build_dir and auspice_dir should not happen. Leave it with us.

mbhall88 avatar Jun 29 '22 02:06 mbhall88

@bricoletc do you think you'll have a chance to look at this? I have added a failing test and opened a draft PR at #151. So maybe add on to that if you can?

It's weird the formatter is passing build_dir and auspice_dir to black separately, but when it gets to auspice_dir it thinks it is still following the configfile keyword I think? I'm a bit lost.

mbhall88 avatar Jun 29 '22 05:06 mbhall88

Hey @mbhall88 i'm afraid i won't be able to cover this before i submit my thesis, so early october. i have to invest all resources into that until then!

bricoletc avatar Jul 11 '22 17:07 bricoletc