Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions nf_core/pipelines/lint/rocrate_readme_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@
def rocrate_readme_sync(self):
"""
Check if the RO-Crate description in ro-crate-metadata.json matches the README.md content.
If the --fix is set, the RO-Crate description will be updated to match the README.md content.
If not, the RO-Crate description will be automatically updated to match the README.md content during linting.
"""

passed = []
failed = []
ignored = []
failed = []
fixed = []
could_fix: bool = False

# Check if the file exists before trying to load it
metadata_file = Path(self.wf_path, "ro-crate-metadata.json")
Expand All @@ -38,6 +37,11 @@ def rocrate_readme_sync(self):
return {"passed": passed, "failed": failed, "ignored": ignored}
readme_content = readme_file.read_text(encoding="utf-8")
graph = metadata_dict.get("@graph")

# Ensure the fix rocrate_readme_sync is in the fix list
if "rocrate_readme_sync" not in self.fix:
self.fix += ("rocrate_readme_sync",)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are not using the --fix flag this can be removed, also the if at line 61

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I think this fix flag is not necessary anymore if we automate the update process. I.e. all the changes would be synchronized anyway during linting (according to my understanding, sorry if I'm wrong about it).

And in this case --fix is still available but always set to true, just in case we want to disable it again someday.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove it completely then, it makes the code more confusing to follow, at least for me 😄


if not graph or not isinstance(graph, list) or not graph[0] or not isinstance(graph[0], dict):
ignored.append("Invalid RO-Crate metadata structure.")
else:
Expand All @@ -54,10 +58,8 @@ def rocrate_readme_sync(self):

# Compare the two strings and add a linting error if they don't match
if readme_content != rc_description_graph:
# If the --fix flag is set, you could overwrite the RO-Crate description with the README content:
if "rocrate_readme_sync" in self.fix:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this check anymore

metadata_dict.get("@graph")[0]["description"] = readme_content
fixed.append("Fixed: add the same description from `README.md` to the RO-Crate metadata.")
with metadata_file.open("w", encoding="utf-8") as f:
json.dump(metadata_dict, f, indent=4)
passed.append("RO-Crate description matches the `README.md`.")
Expand All @@ -66,7 +68,6 @@ def rocrate_readme_sync(self):
failed.append(
"The RO-Crate descriptions do not match the README.md content. Use `nf-core pipelines lint --fix rocrate_readme_sync` to update."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update the error to remove mention to --fix flag

)
could_fix = True
else:
passed.append("RO-Crate descriptions are in sync with `README.md`.")
return {"passed": passed, "failed": failed, "ignored": ignored, "fixed": fixed, "could_fix": could_fix}
return {"passed": passed, "failed": failed, "ignored": ignored, "fixed": fixed}
27 changes: 1 addition & 26 deletions tests/pipelines/lint/test_rocrate_readme_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,6 @@ def test_rocrate_readme_sync_pass(self):
assert len(results.get("failed", [])) == 0
assert len(results.get("passed", [])) > 0

def test_rocrate_readme_sync_fail(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is still valid, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it because there won't be any "failed" cases anymore👀 shall I always keep the tests for all cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have the error (at rocrate_readme_sync.py line 66), so I would keep the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There won't be failed cases if I remove all the checks such as if "rocrate_readme_sync" in self.fix: ..., so I guess this test would anyway be not necessary anymore?

self.lint_obj._load()

json_path = Path(self.lint_obj.wf_path, "ro-crate-metadata.json")
with open(json_path) as f:
try:
rocrate = json.load(f)
except json.JSONDecodeError as e:
raise UserWarning(f"Unable to load JSON file '{json_path}' due to error {e}")
rocrate["@graph"][0]["description"] = "This is a test script"
with open(json_path, "w") as f:
json.dump(rocrate, f, indent=4)
results = self.lint_obj.rocrate_readme_sync()
assert len(results.get("failed", [])) == 1
assert (
"The RO-Crate descriptions do not match the README.md content. Use `nf-core pipelines lint --fix rocrate_readme_sync` to update."
in results.get("failed", [])
)

def test_rocrate_readme_sync_fixed(self):
self.lint_obj._load()
json_path = Path(self.lint_obj.wf_path, "ro-crate-metadata.json")
Expand All @@ -43,12 +24,6 @@ def test_rocrate_readme_sync_fixed(self):
with open(json_path, "w") as f:
json.dump(rocrate, f, indent=4)

results = self.lint_obj.rocrate_readme_sync()
assert len(results.get("failed", [])) == 1

# Fix the issue
assert "rocrate_readme_sync" in self.lint_obj.lint_tests
self.lint_obj.fix = ["rocrate_readme_sync"]
self.lint_obj._load()
results = self.lint_obj.rocrate_readme_sync()
assert len(results.get("failed", [])) == 0
assert len(results.get("fixed", [])) == 1