Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always format modules.json after writing it #2074

Merged
merged 16 commits into from
Dec 5, 2022

Conversation

fabianegli
Copy link
Contributor

@fabianegli fabianegli commented Nov 29, 2022

Always format modules.json after writing it.

Closes #2047

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

@fabianegli fabianegli added the WIP Work in progress label Nov 29, 2022
@fabianegli
Copy link
Contributor Author

It seems that some tests (TestModules.test_modules_lint_patched_modules) don't set up a git repo but write the modules.json file.

@awgymer
Copy link
Contributor

awgymer commented Nov 29, 2022

It seems that some tests (TestModules.test_modules_lint_patched_modules) don't set up a git repo but write the modules.json file.

Yes. All the modules/subworkflows setup initialises it's test pipeline repo without git:

nf_core.create.PipelineCreate(
            "mypipeline", "it is mine", "me", no_git=True, outdir=self.pipeline_dir, plain=True
        ).init_pipeline()

I guess this is the unforseen side-effect of routing everything through pre-commit.
To be honest isn't the point of a pre-commit hook that it can be configured so it runs before a commit?
Running formatting every single time you edit a file - which may or may not - ever be committed to git seems like overkill?

@awgymer
Copy link
Contributor

awgymer commented Nov 29, 2022

I'm actually surprised this didn't break the tests in the original PR to add this, but I am guessing none of our tests create a non-nf-core branded repo (which is also non-git), which is the only situation it was used in before.

@fabianegli
Copy link
Contributor Author

fabianegli commented Nov 29, 2022

The tests for the function set up a git repo, create a file and run the hook on it. These tests are independent from how everything else in nf-core tools, as they should be. That was creating a capability and making sure it is working as expected.

Integrating a new functionality in an often used method like dump is not unlikely to raise some flags. In the current failing test it will be mostly a question of making sure the the test actually runs in a git repo. Which is a valid expectation for any module because git is at the core of it all.

@awgymer
Copy link
Contributor

awgymer commented Nov 29, 2022

The tests for the function are what they are and they all pass, but for whatever reason there is a provision to create nf-core repos without git. How much intention there is to support that I don't know, but given that, we have un-tested functionality which was broken by moving to use pre-commit. It is now not possible to successfully create a non-git repo despite it being offered by the tools:

❯ nf-core create

                                          ,--./,-.
          ___     __   __   __   ___     /,-._.--~\
    |\ | |__  __ /  ` /  \ |__) |__         }  {
    | \| |       \__, \__/ |  \ |___     \`-._,-`-,
                                          `._,._,'

    nf-core/tools version 2.7.dev0 - https://nf-co.re


? Workflow name test
? Description a test
? Author me
? Do you want to customize which parts of the template are used? Yes
? Pipeline prefix
ERROR    Pipeline prefix cannot start with digit or hyphen and cannot contain punctuation.                                                                                                               
? Please provide a new pipeline prefix dummy
? Skip template areas? done (5 selections)
INFO     Creating new nf-core pipeline: 'dummy/test'                                                                                                                                                     
╭───────────────────────────────────────────────────────────────────────────────── Traceback (most recent call last) ──────────────────────────────────────────────────────────────────────────────────╮
│ /Users/arthurgymer/workbench/nf-core/nf-core-tools/nf_core/lint_utils.py:68 in run_prettier_on_file                                                                                                  │
│                                                                                                                                                                                                      │
│   67 │   try:                                                                                                                                                                                        │
│ ❱ 68 │   │   subprocess.run(                                                                                                                                                                         │
│   69 │   │   │   ["pre-commit", "run", "--config", nf_core_pre_commit_config, "prettier",                                                                                                            │
│      "--files", file],                                                                                                                                                                               │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/.pyenv/versions/3.9.7/lib/python3.9/subprocess.py:528 in run                                                                                                                      │
│                                                                                                                                                                                                      │
│    527 │   │   if check and retcode:                                                                                                                                                                 │
│ ❱  528 │   │   │   raise CalledProcessError(retcode, process.args,                                                                                                                                   │
│    529 │   │   │   │   │   │   │   │   │    output=stdout, stderr=stderr)                                                                                                                            │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
CalledProcessError: Command '['pre-commit', 'run', '--config', PosixPath('/Users/arthurgymer/workbench/nf-core/nf-core-tools/.pre-commit-config.yaml'), 'prettier', '--files', 
PosixPath('/Users/arthurgymer/workbench/nf-core/dummy-test/nextflow_schema.json')]' returned non-zero exit status 1.

The above exception was the direct cause of the following exception:

╭───────────────────────────────────────────────────────────────────────────────── Traceback (most recent call last) ──────────────────────────────────────────────────────────────────────────────────╮
│ /Users/arthurgymer/.pyenv/versions/nf-core-dev/bin/nf-core:8 in <module>                                                                                                                             │
│                                                                                                                                                                                                      │
│   7 │   sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])                                                                                                                             │
│ ❱ 8 │   sys.exit(run_nf_core())                                                                                                                                                                      │
│   9                                                                                                                                                                                                  │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/workbench/nf-core/nf-core-tools/nf_core/__main__.py:104 in run_nf_core                                                                                                            │
│                                                                                                                                                                                                      │
│    103 │   # Launch the click cli                                                                                                                                                                    │
│ ❱  104 │   nf_core_cli(auto_envvar_prefix="NFCORE")                                                                                                                                                  │
│    105                                                                                                                                                                                               │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/.pyenv/versions/3.9.7/envs/nf-core-dev/lib/python3.9/site-packages/click/core.py:1130 in __call__                                                                                 │
│                                                                                                                                                                                                      │
│   1129 │   │   """Alias for :meth:`main`."""                                                                                                                                                         │
│ ❱ 1130 │   │   return self.main(*args, **kwargs)                                                                                                                                                     │
│   1131                                                                                                                                                                                               │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/.pyenv/versions/3.9.7/envs/nf-core-dev/lib/python3.9/site-packages/rich_click/rich_group.py:21 in main                                                                            │
│                                                                                                                                                                                                      │
│   20 │   │   try:                                                                                                                                                                                    │
│ ❱ 21 │   │   │   rv = super().main(*args, standalone_mode=False, **kwargs)                                                                                                                           │
│   22 │   │   │   if not standalone_mode:                                                                                                                                                             │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/.pyenv/versions/3.9.7/envs/nf-core-dev/lib/python3.9/site-packages/click/core.py:1055 in main                                                                                     │
│                                                                                                                                                                                                      │
│   1054 │   │   │   │   with self.make_context(prog_name, args, **extra) as ctx:                                                                                                                      │
│ ❱ 1055 │   │   │   │   │   rv = self.invoke(ctx)                                                                                                                                                     │
│   1056 │   │   │   │   │   if not standalone_mode:                                                                                                                                                   │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/.pyenv/versions/3.9.7/envs/nf-core-dev/lib/python3.9/site-packages/click/core.py:1657 in invoke                                                                                   │
│                                                                                                                                                                                                      │
│   1656 │   │   │   │   with sub_ctx:                                                                                                                                                                 │
│ ❱ 1657 │   │   │   │   │   return _process_result(sub_ctx.command.invoke(sub_ctx))                                                                                                                   │
│   1658                                                                                                                                                                                               │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/.pyenv/versions/3.9.7/envs/nf-core-dev/lib/python3.9/site-packages/click/core.py:1404 in invoke                                                                                   │
│                                                                                                                                                                                                      │
│   1403 │   │   if self.callback is not None:                                                                                                                                                         │
│ ❱ 1404 │   │   │   return ctx.invoke(self.callback, **ctx.params)                                                                                                                                    │
│   1405                                                                                                                                                                                               │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/.pyenv/versions/3.9.7/envs/nf-core-dev/lib/python3.9/site-packages/click/core.py:760 in invoke                                                                                    │
│                                                                                                                                                                                                      │
│    759 │   │   │   with ctx:                                                                                                                                                                         │
│ ❱  760 │   │   │   │   return __callback(*args, **kwargs)                                                                                                                                            │
│    761                                                                                                                                                                                               │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/workbench/nf-core/nf-core-tools/nf_core/__main__.py:295 in create                                                                                                                 │
│                                                                                                                                                                                                      │
│    294 │   │   )                                                                                                                                                                                     │
│ ❱  295 │   │   create_obj.init_pipeline()                                                                                                                                                            │
│    296 │   except UserWarning as e:                                                                                                                                                                  │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/workbench/nf-core/nf-core-tools/nf_core/create.py:235 in init_pipeline                                                                                                            │
│                                                                                                                                                                                                      │
│   234 │   │   # Make the new pipeline                                                                                                                                                                │
│ ❱ 235 │   │   self.render_template()                                                                                                                                                                 │
│   236                                                                                                                                                                                                │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/workbench/nf-core/nf-core-tools/nf_core/create.py:337 in render_template                                                                                                          │
│                                                                                                                                                                                                      │
│   336 │   │   if not self.template_params["igenomes"] or not                                                                                                                                         │
│       self.template_params["nf_core_configs"]:                                                                                                                                                       │
│ ❱ 337 │   │   │   self.update_nextflow_schema()                                                                                                                                                      │
│   338                                                                                                                                                                                                │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/workbench/nf-core/nf-core-tools/nf_core/create.py:364 in update_nextflow_schema                                                                                                   │
│                                                                                                                                                                                                      │
│   363 │   │   schema.save_schema(suppress_logging=True)                                                                                                                                              │
│ ❱ 364 │   │   run_prettier_on_file(schema_path)                                                                                                                                                      │
│   365                                                                                                                                                                                                │
│                                                                                                                                                                                                      │
│ /Users/arthurgymer/workbench/nf-core/nf-core-tools/nf_core/lint_utils.py:76 in run_prettier_on_file                                                                                                  │
│                                                                                                                                                                                                      │
│   75 │   │   │   raise ValueError(f"Can't format {file} because it has a synthax                                                                                                                     │
│      error.\n{e.stdout.decode()}") from e                                                                                                                                                            │
│ ❱ 76 │   │   raise ValueError(                                                                                                                                                                       │
│   77 │   │   │   "There was an error running the prettier pre-commit hook.\n"                                                                                                                        │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
ValueError: There was an error running the prettier pre-commit hook.
STDOUT: An error has occurred: FatalError: git failed. Is it installed, and are you in a Git repository directory?
Check the log at /Users/arthurgymer/.cache/pre-commit/pre-commit.log

STDERR: 

We could simply revert to the old behaviour of an exception during prettier invocation not being fatal? So instead of raising a ValueError just log it and move on silently? There are no use-cases in which an unformatted file should be fatal I think?

@fabianegli
Copy link
Contributor Author

fabianegli commented Nov 30, 2022

@awgymer Thank you for all the testing and feedback! Moving from a failure to a warning is probably reasonable and I'll update the PR accordingly.

The merge of the pre-commit prettier PR came as a bit of a surprise to me as it was still labeled as WIP. But since it has been merged the next logical step is to test ride it and see how it plays with the rest of the code and how it impacts UX. Again, a WIP, because I am not sure where this PR is going ...

The non-git workflow generation might be remnant of the past - introduced before git was used under the hood for a lot of the functionality. I wonder if it still has valid use cases and if it should be deprecated in the future?

@mashehu
Copy link
Contributor

mashehu commented Nov 30, 2022

Awesome, wanted to work on this today, because this will fix #2047.

Sorry for the 🤠-merge of #1983, we were thinking it was more or less done.

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #2074 (c98c5c7) into dev (9974e0d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #2074      +/-   ##
==========================================
+ Coverage   68.00%   68.01%   +0.01%     
==========================================
  Files          43       43              
  Lines        5620     5625       +5     
==========================================
+ Hits         3822     3826       +4     
- Misses       1798     1799       +1     
Impacted Files Coverage Δ
nf_core/lint_utils.py 85.71% <100.00%> (ø)
nf_core/modules/lint/__init__.py 83.24% <0.00%> (-0.27%) ⬇️
nf_core/__main__.py 58.65% <0.00%> (+0.13%) ⬆️

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

@fabianegli
Copy link
Contributor Author

I was not much opposed to the merge, just a bit surprised about the speed 🚀

@ewels
Copy link
Member

ewels commented Nov 30, 2022

Sorry @fabianegli - my fault for the fast merge 😬 I saw the WIP label but assumed it was stale.

Fine by me to remove the functionality to create without a git repo, I'm not sure that this is used much and a manual workaround is trivial.

Agree that prettier errors should trigger a warning only and not a failure 👍🏻

@fabianegli
Copy link
Contributor Author

Agree that prettier errors should trigger a warning only and not a failure 👍🏻

Does that apply to syntax errors as well? They are currently raising a ValueError. And I think they should because this should never happen.

@fabianegli fabianegli removed the WIP Work in progress label Nov 30, 2022
@ewels
Copy link
Member

ewels commented Nov 30, 2022

And I think they should because this should never happen.

The downside of killing execution with an error is that it often leaves things in a half-done state. For example, modules half installed or so on. In my experience it's usually easier to log an error and keep going, then let the person manually run Prettier themselves afterwards and fix whatever the problem was themselves.

"It shouldn't happen" is very far from "it doesn't happen" 😅

@fabianegli
Copy link
Contributor Author

It's not that I disagree with that sentiment. Half done things are bad.

... but prettier will never be able to fix syntax errors 🤷

If this happens I'd expect an issue to pop up here to make the devs figure out what's going awry and fix it.

@awgymer
Copy link
Contributor

awgymer commented Nov 30, 2022

They are going to have to fix it manually regardless. By exiting early with an error - instead of just logging it (as critical if you want) and printing a big message to explain they need to fix it - you risk breaking a bunch of other stuff which they may have to rectify manually but won't necessarily even be reported as broken.

nf_core/lint_utils.py Outdated Show resolved Hide resolved
awgymer and others added 2 commits November 30, 2022 14:16
Change prettier syntax errors to issue a log instead of error out

Co-authored-by: Fabian Egli <fabianegli@users.noreply.github.com>
@fabianegli
Copy link
Contributor Author

Now the tests checking for the ValueError on syntax error will fail and need fixing...

tests/test_lint_utils.py Outdated Show resolved Hide resolved
@fabianegli fabianegli added the WIP Work in progress label Dec 5, 2022
tests/test_lint_utils.py Outdated Show resolved Hide resolved
Co-authored-by: awgymer <24782660+awgymer@users.noreply.github.com>
tests/test_lint_utils.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants