Give better suggestion when modules.json file is out-of-date. #1741

mashehu · 2022-08-05T15:12:09Z

Running nf-core modules info never recreates the modules.json. So, if it is out-of-date, the suggested help message is not useful. I think explicitly stating a command is more useful here.

…x-suggestion # Conflicts: # nf_core/modules/modules_json.py

codecov · 2022-08-05T15:18:34Z

Codecov Report

Merging #1741 (4d608ae) into dev (9c8614b) will increase coverage by 0.08%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##              dev    #1741      +/-   ##
==========================================
+ Coverage   69.14%   69.22%   +0.08%     
==========================================
  Files          59       59              
  Lines        7133     7142       +9     
==========================================
+ Hits         4932     4944      +12     
+ Misses       2201     2198       -3

Impacted Files	Coverage Δ
nf_core/modules/list.py	`71.83% <ø> (+0.59%)`	⬆️
nf_core/modules/modules_command.py	`83.56% <ø> (-0.23%)`	⬇️
nf_core/modules/modules_json.py	`74.65% <95.00%> (+1.28%)`	⬆️
nf_core/lint/__init__.py	`74.07% <100.00%> (+0.10%)`	⬆️
nf_core/modules/remove.py	`82.75% <100.00%> (+0.61%)`	⬆️

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

mashehu · 2022-08-05T15:24:27Z

@nf-core-bot fix linting

ErikDanielsson · 2022-08-05T15:29:09Z

I think that a better solution would be to call the ModulesJson.has_modules_file from all commands that use the modules.json -- there is no need to have the user run another command. We could even call it from the ModulesJson.check_up_to_date that checks whether the modules.json correspond to the modules present in the pipeline

…json file run also `check_up_to_date`, which now rebuilds outdates files

…x-suggestion

into better-fix-suggestion # Conflicts: # nf_core/modules/modules_json.py

ErikDanielsson · 2022-08-05T16:46:17Z

nf_core/modules/modules_json.py

        self.load()
        if not self.has_git_url_and_base_path():
-            raise UserWarning(
-                "The 'modules.json' file is not up to date. " "You can fix it by running 'nf-core modules update'."
-            )
+            log.info("The 'modules.json' file is not up to date. Recreating the 'module.json' file.")
+            self.create()
+            self.load()


I'd say it would be better with a try catch block around the call to self.load that catches the LookupError that it will throw if the file does not exist. You could also assign the modules_json variable to self.modules_json in create and skip the second call to load since there should be no need to go via the file.

Good idea, changed it accordingly.
Now I am actually wondering, if we need all of the

if self.modules_json is None: self.load()

in the different submethods, when they are (as far as I saw) all called after a check_up_to_date() call. Or is that just to be extra sure for future methods?

Sounds good. Just grep for the creation of ModulesJson objects and make sure that they are not created without check_up_to_date being called. This will atleast be true for some tests.

I think I will actually leave it for now. Too many possible downstream effects, I might not be aware of now. But I added a test for .check_up_to_date(), so this PR should be good to go. 🙂

…reate()`

…x-suggestion

ErikDanielsson

This is starting to look good!

nf_core/modules/list.py

ErikDanielsson · 2022-08-09T19:00:38Z

nf_core/modules/modules_command.py

            raise UserWarning(f"Could not find a 'main.nf' or 'nextflow.config' file in '{self.dir}'")
-        self.has_modules_file()
        return True


I'm not sure I'm following why you removed this line, and added it as standalone calls (:

we discussed this on slack: all commands that used this function use check_up_to_date anyway, except for linting. So I kept it there.

…e check for missing `git_sha` and `branch` fields in it

nf_core/modules/modules_json.py

…x-suggestion

…path) and return booleans instead of warnings

nf_core/modules/modules_json.py

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

mirpedrol

After #1754 is merged, there are some conflicts to resolve. Apart from that, if the comments from other reviewers are solved, it looks good to me :)

nf_core/modules/modules_json.py

mirpedrol · 2022-08-22T07:10:13Z

tests/modules/modules_json.py

+def test_mod_json_with_missing_base_path_entry(self):
+    # Load module.json and remove the base_path entry
+    mod_json_obj = ModulesJson(self.pipeline_dir)
+    mod_json_orig = mod_json_obj.get_modules_json()
+    mod_json = copy.deepcopy(mod_json_orig)
+    mod_json["repos"]["nf-core/modules"].pop("base_path")
+    # save the altered module.json and load it again to check if it will fix itself
+    mod_json_obj.modules_json = mod_json
+    mod_json_obj.dump()
+    mod_json_obj_new = ModulesJson(self.pipeline_dir)
+    mod_json_obj_new.check_up_to_date()
+    mod_json_new = mod_json_obj_new.get_modules_json()
+    assert mod_json_orig == mod_json_new


I think this test can also be removed after removing base_path

will update them to test with another entry. They should just make sure to be able to fix a broken modules.json

tests/modules/modules_json.py

tests/test_modules.py

…x-suggestion # Conflicts: # CHANGELOG.md # nf_core/modules/modules_json.py

.github/workflows/tools-api-docs-dev.yml

…x-suggestion # Conflicts: # .github/workflows/tools-api-docs-dev.yml # .github/workflows/tools-api-docs-release.yml

into better-fix-suggestion

mashehu added 3 commits August 5, 2022 17:04

Give better suggestion when modules.json file is out-of-date.

d445eeb

Merge branch 'dev' of https://github.com/nf-core/tools into better-fi…

0e3560a

…x-suggestion # Conflicts: # nf_core/modules/modules_json.py

change message in new commands

1f1ff4f

mashehu requested a review from ErikDanielsson August 5, 2022 15:12

[automated] Fix linting with Prettier

f7dbfe4

mashehu added 4 commits August 5, 2022 18:12

recreate modules.json instead of complaining

a7a7b29

remove has_module_file method, as all commands relying on a module.…

d600c8f

…json file run also `check_up_to_date`, which now rebuilds outdates files

Merge branch 'dev' of https://github.com/nf-core/tools into better-fi…

e90c406

…x-suggestion

Merge branch 'better-fix-suggestion' of https://github.com/mashehu/tools

7227f77

into better-fix-suggestion # Conflicts: # nf_core/modules/modules_json.py

ErikDanielsson reviewed Aug 5, 2022

View reviewed changes

mashehu added 7 commits August 8, 2022 11:26

add has_modules_file() back in for lint and remove command

dc2408c

Improve error handling, assign modules_json variable already in `.c…

6dd5b8b

…reate()`

add test for module.json with incorrect format

fc7693e

Merge branch 'dev' of https://github.com/nf-core/tools into better-fi…

4db1ed2

…x-suggestion

fix test for incorrect mod_json

ca39fb9

fix isort

6d3717a

updated changelog

47d551b

ErikDanielsson reviewed Aug 9, 2022

View reviewed changes

rename has_git_url_and_base_path to has_correct_format and includ…

5c05171

…e check for missing `git_sha` and `branch` fields in it

fabianegli reviewed Aug 17, 2022

View reviewed changes

nf_core/modules/modules_json.py Show resolved Hide resolved

mashehu added 3 commits August 18, 2022 15:14

Merge branch 'dev' of https://github.com/nf-core/tools into better-fi…

3c07afb

…x-suggestion

rename to has_git_url_and_modules (in expectation of removal of base_…

3f209ab

…path) and return booleans instead of warnings

fix isort error

6ca8836

fabianegli reviewed Aug 18, 2022

View reviewed changes

nf_core/modules/modules_json.py Outdated Show resolved Hide resolved

fabianegli reviewed Aug 18, 2022

View reviewed changes

nf_core/modules/modules_json.py Outdated Show resolved Hide resolved

Apply suggestions from code review

c743e84

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

mashehu added this to the 2.5 milestone Aug 22, 2022

mirpedrol approved these changes Aug 22, 2022

View reviewed changes

nf-core_bot added 5 commits August 22, 2022 15:25

Merge branch 'dev' of https://github.com/nf-core/tools into better-fi…

04c79d4

…x-suggestion # Conflicts: # CHANGELOG.md # nf_core/modules/modules_json.py

fix isort

b1f4501

update ubuntu version for docs GHA

e2f50c4

update changelog

863c390

fix typo

db147b4

ewels reviewed Aug 23, 2022

View reviewed changes

.github/workflows/tools-api-docs-dev.yml Outdated Show resolved Hide resolved

mashehu and others added 5 commits August 23, 2022 13:14

Merge branch 'dev' into better-fix-suggestion

c9ea870

Merge branch 'dev' of https://github.com/nf-core/tools into better-fi…

ef9474e

…x-suggestion # Conflicts: # .github/workflows/tools-api-docs-dev.yml # .github/workflows/tools-api-docs-release.yml

Merge branch 'better-fix-suggestion' of https://github.com/mashehu/tools

b042c18

into better-fix-suggestion

update changelog

364e240

add link to PR

4d608ae

mashehu merged commit 20ad2aa into nf-core:dev Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give better suggestion when modules.json file is out-of-date. #1741

Give better suggestion when modules.json file is out-of-date. #1741

mashehu commented Aug 5, 2022

codecov bot commented Aug 5, 2022 •

edited

Loading

mashehu commented Aug 5, 2022

ErikDanielsson commented Aug 5, 2022

ErikDanielsson Aug 5, 2022 •

edited

Loading

mashehu Aug 8, 2022

ErikDanielsson Aug 8, 2022

mashehu Aug 9, 2022

ErikDanielsson left a comment

ErikDanielsson Aug 9, 2022 •

edited

Loading

mashehu Aug 18, 2022

mirpedrol left a comment

mirpedrol Aug 22, 2022

mashehu Aug 22, 2022

Give better suggestion when modules.json file is out-of-date. #1741

Give better suggestion when modules.json file is out-of-date. #1741

Conversation

mashehu commented Aug 5, 2022

codecov bot commented Aug 5, 2022 • edited Loading

Codecov Report

mashehu commented Aug 5, 2022

ErikDanielsson commented Aug 5, 2022

ErikDanielsson Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

mashehu Aug 8, 2022

Choose a reason for hiding this comment

ErikDanielsson Aug 8, 2022

Choose a reason for hiding this comment

mashehu Aug 9, 2022

Choose a reason for hiding this comment

ErikDanielsson left a comment

Choose a reason for hiding this comment

ErikDanielsson Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

mashehu Aug 18, 2022

Choose a reason for hiding this comment

mirpedrol left a comment

Choose a reason for hiding this comment

mirpedrol Aug 22, 2022

Choose a reason for hiding this comment

mashehu Aug 22, 2022

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2022 •

edited

Loading

ErikDanielsson Aug 5, 2022 •

edited

Loading

ErikDanielsson Aug 9, 2022 •

edited

Loading