run prettier on test yml file after creation

[fabianegli]

Closes #1866

ToDos

• Sort out the disconnect of what is shown in the CLI output and what is written to the yaml-file & prettified.

PR checklist

• This comment contains a description of changes (with reason)
• CHANGELOG.md is updated
• If you've fixed a bug or added code that should be tested, add tests!
• Documentation in docs is updated

[codecov]

Codecov Report

Merging #2078 (3d9329b) into dev (da49046) will increase coverage by 0.02%.
The diff coverage is 42.30%.

@@            Coverage Diff             @@
##              dev    #2078      +/-   ##
==========================================
+ Coverage   67.98%   68.01%   +0.02%     
==========================================
  Files          43       43              
  Lines        5597     5605       +8     
==========================================
+ Hits         3805     3812       +7     
- Misses       1792     1793       +1     

Impacted Files	Coverage Δ
nf_core/subworkflows/test_yml_builder.py	15.28% <7.69%> (+0.16%)	⬆️
nf_core/modules/test_yml_builder.py	51.33% <76.92%> (+1.79%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mashehu]

There is now a disconnect of what is shown in the CLI output and what is written to the yaml-file&prettified

What if we read it from the yaml file and then print that?

[fabianegli]

What if we read it from the yaml file and then print that?

It introduces additional steps, but is ultimately necessary to get the desired consistency. It is a question though how to go about that exactly - because it needs a temporary file that can be formatted with pre-commit which requires a git repo, but then again we don't want to pollute a git repo with a file we don't want to keep. So there will need to be a viable temporary file procedure. Can't put much more time into this today - code contributions are more than welcome.

[awgymer]

FWIW by the way, the underlying issue isn't actually an issue anymore.
It was fixed by the original YAML Dumper fix which was mentioned in the other issue which started the move to extracting the prettier files :)

Example output of create-test-yml from the current dev branch.

- name: cat fastq test_cat_fastq_single_end
  command: nextflow run ./tests/modules/nf-core/cat/fastq -entry test_cat_fastq_single_end -c ./tests/config/nextflow.config -c ./tests/modules/nf-core/cat/fastq/nextflow.config
  tags:
    - cat
    - cat/fastq
  files:
    - path: output/cat/test.merged.fastq.gz
      md5sum: f9cf5e375f7de81a406144a2c70cc64d
    - path: output/cat/versions.yml

- name: cat fastq test_cat_fastq_paired_end
  command: nextflow run ./tests/modules/nf-core/cat/fastq -entry test_cat_fastq_paired_end -c ./tests/config/nextflow.config -c ./tests/modules/nf-core/cat/fastq/nextflow.config
  tags:
    - cat
    - cat/fastq
  files:
    - path: output/cat/test_1.merged.fastq.gz
      md5sum: f9cf5e375f7de81a406144a2c70cc64d
    - path: output/cat/test_2.merged.fastq.gz
      md5sum: 77c8e966e130d8c6b6ec9be52fcb2bda
    - path: output/cat/versions.yml

[fabianegli]

The goal of this whole exercise would also be to make the custom yaml dumper simpler or remove it entirely.

[fabianegli]

I try to go step by step - it's normally faster in the end, safer to do and doesn't produce giant PRs.

[fabianegli]

The way I like to think about this adventure is that it will result in a better separation of concerns. The the yaml dumper dumps and the formatter formats.

[awgymer]

I think a belt-and-braces approach of having both working is better to be honest.
If you are really wedded to ensuring the CLI output has also been run through prettier (if they save it to file they'll just need to run it again though no?) then I would suggest maybe:

with tempfile.NamedTemporaryFile(mode='w+') as fh:
    yaml.dump(self.tests, fh, Dumper=nf_core.utils.custom_yaml_dumper(), width=10000000)
    run_prettier_on_file(fh.name)
    prettified_yml = fh.read()
if self.test_yml_output_path == "-":
    console = rich.console.Console()
    console.print("\n", Syntax(yaml_str, "yaml"), "\n")
else:
    try:
        log.info(f"Writing to '{self.test_yml_output_path}'")
        with open(self.test_yml_output_path, "w") as fh:
            fh.write(prettified_yml)
    except FileNotFoundError as e:
        raise UserWarning(f"Could not create test.yml file: '{e}'")

[fabianegli]

@awgymer If you'd like you can push the code snippet to this PR branch yourself. (I think nf-core contributors have the necessary permissions to directly push to my branches.)

[awgymer]

Ok that appears to be passing the tests. Does somebody want to test it in the wild?

[mashehu]

Somehow the fh.read() always returns just an empty string when testing (but the file is there and nicely formatted).

[awgymer]

Somehow the fh.read() always returns just an empty string when testing (but the file is there and nicely formatted).

I'm confused by what you mean? How do you know it's empty if the (I assume final) file is written?

[mashehu]

How do you know it's empty if the (I assume final) file is written?

Both the console output is just an empty line and when I step into it with ipdb, I see that self.tests is fine, but fh.read() returns ''

[fabianegli]

@mashehu Good catch! We're missing a fh.seek(0) before the read.

[awgymer]

I think this is good to go now. Who should review/approve as we have all contirbuted code now 😂

[fabianegli]

@ewels ? 😅

[ewels]

Nitpick: Can we not just pass stdin to the prettier CLI and avoid messing around with temporary files?

https://prettier.io/docs/en/cli.html#--stdin-filepath

[awgymer]

Nitpick: Can we not just pass stdin to the prettier CLI and avoid messing around with temporary files?

https://prettier.io/docs/en/cli.html#--stdin-filepath

I did think this at first. But I am fairly certain when I tested it prettier requires that filepath to actually exist. Which made me question what even the point of it is

[fabianegli]

pre-commit intentionally hides the CLI of the hooks. It is a central part of the philosophy of having reproducible hooks. This means there is no reasonable way to add CLI flags and hence we have to submit files to the pre-commit hook.

[fabianegli]

It is a bit of a detour, but it is one that is easily understood and the complications don't snowball or stretch into other parts of the code - AFAICT.

[awgymer]

Something that just struck me as we are hacking around pre-commit.

If we are running pre-commit install (which I assume we must be to get prettier) does that not mean that the hooks will run when people commit anyway - which is the only point at which the formatting actually matters?

And would we solve all this by just running pre-commit install as part of pipeline creation? (We are already enforcing it as a dependency at this point)

[fabianegli]

The pre-commit hook config in tools is used in different places. The following is a list sorted in ascending distance to the invocation of nf_core that creates a file:

1. As part of the autogeneration of files to ensure the files nf_core produces are formatted corrected.
2. As pre-commit hook to prevent malformed content to enter the repository.
3. In the CI (nf-core/tools & modules) provided a convenience to the devs as

The reason for 1. is that Prettier is not pip-installable and we need a robust and convenient way to ensure it is available - pre-commit does that.

The reason for 2. is convenience for the developers - faster feedback. Not everyone will actually install the pre-commit hooks in their local git repo, so we can't reasonably expect that files are actually being checked at commit time. (pre-commit installs the script on one system - everyone cloning the repo has to install it again separately in their clone of the repo.)

And 3. ensures quality at the community level.

With pre-commit we can guarantee consistency on all levels. This is the value proposition of using pre-commit for all of this.

[awgymer]

But doesn't tools - by using pre-commit - actually end up doing number 2 whether they want it or not? There is a call to pre-commit install in the code now isn't there?

That was the whole reason we need to now enforce that repos are git repos? And actually I think this may now be broken if any of the commands using prettier are passed a directory other than "." i.e. current working directory. (Currently I think there are some commands that you can point to a dir but we don't actually set it as the working dir where prettier would run, but I would need to have a proper look at the code)

[fabianegli]

We might want to change that install subcommand to install-hooks instead. To grab the capability upon install, but not force the pre-commit hooks on the devs. Or do we? pre-commit can work without the scripts being installed into the git repo to run the hooks before a commit.

In general, yes, this whole pre-commit might still have a bunch of corner cases and the more use cases we can test for the better. So please, if you know a use case you think might break please write it down and/or try it out. And if possible write a test for it.

[mashehu]

To grab the capability upon install, but not force the pre-commit hooks on the devs

That is my desired functionality for this: don't force git hooks on people, but use it as a way to get prettier. But I don't actually see that we run "prettier install" anywhere or am I missing something.

Anyway, this PR does work for me now, so I'll give it my 👍🏻 so we can merge it. 🙂

[mirpedrol]

I added the same code for subworkflows. It would be ideal to refactor this command with a components class as we did for the others, but I would leave like this for this PR.
Everyone happy with merging?

[awgymer]

But I don't actually see that we run "prettier install" anywhere or am I missing something.

If that is the case then I think we are fine. I thought I remembered seeing it but maybe it was in an early draft of adding prettier via precommit

[fabianegli]

I don't actually see that we run "prettier install" anywhere or am I missing something.

NB: I meant pre-commit install and pre-commit install-hooks. But since it's nowhere I think we can forget about it for now.