Get name for parent path to create schema_filename

[alexhermida]

Hi 👋🏽 I've came to fix an issue but didn't realise about the dev branch until PR time, I've missed that detail in CONTRIBUTORS file, so the issue was already fixed with the str cast doing the trick.

I kept the PR as I think using as_posix is generally more correct for Paths, but maybe I'm missing something for this use case. First time around here.

Also adding a test since it wasn't tested the particular error.

Feel free to drop the PR in case is not necessary.

Thanks.

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

[awgymer]

I was not sure this should really matter too much but now I am having another realisation. This code probably isn't doing what anyone thinks it is doing. And it probably never has.

In the most common case, where you run nf-core schema build then dir defaults to . which means that schema_filename defaults to ./nextflow_schema.json. Which means Path(self.schema_filename).parent resolves to . (which is also true for the previous os.path.dirname implementation. This means that the pipeline name would be set to ..

My suspicion is that the actual intention of this code is to set the pipeline name to be the same as the name of the parent directory e.g. /path/to/my_pipeline/main.nf would result in the pipeline name being set to my_pipeline.

Even if this code operated on the absolute path right now (or previously) it would actually be set to /path/to/my_pipeline

@mashehu you implemented the original code I think, so do you have a take on what the intended outcome was?

[alexhermida]

I was not sure this should really matter too much but now I am having another realisation. This code probably isn't doing what anyone thinks it is doing. And it probably never has.

In the most common case, where you run nf-core schema build then dir defaults to . which means that schema_filename defaults to ./nextflow_schema.json. Which means Path(self.schema_filename).parent resolves to . (which is also true for the previous os.path.dirname implementation. This means that the pipeline name would be set to ..

My suspicion is that the actual intention of this code is to set the pipeline name to be the same as the name of the parent directory e.g. /path/to/my_pipeline/main.nf would result in the pipeline name being set to my_pipeline.

Even if this code operated on the absolute path right now (or previously) it would actually be set to /path/to/my_pipeline

@mashehu you implemented the original code I think, so do you have a take on what the intended outcome was?

Thanks for the comment @awgymer

When I came to address the issue my though was to control and raise an AssertionError as it's in other areas of the code for the schema validation.
Like you I don't know of the background to set this to the parent path but the reasoning you commented it makes sense to me. Personally, I'd prefer to validate and force to explicitly set a pipeline name but maybe that's not consistent what it's done in other parts of the codebase.
If the latter is the case I think something like

Path(self.schema_filename).parent.name).strip("'")

would do the trick.

Let see what @mashehu says. Thank you!

[mashehu]

oooh, good catch. yes, we should probably always convert to a full path

[alexhermida]

I've updated the tests for covering the issue.

The wc is currently hardcoded as it is being created during initialisation. I can see that string is hardcoded in a few places, maybe is worth to refactor in a following PR

tools/tests/test_schema.py

Line 31 in 9eac5e9

self.template_dir = os.path.join(self.tmp_dir, "wf")

[awgymer]

I think the hardcoding in this case is probably fine. We are hardcoding the input path and the expected value based on that?

Only downside I really see there is that people might wonder how we know what the expected is if they don't realise we hardcode the input path but... 🤷

[awgymer]

@nf-core-bot fix linting

[alexhermida]

I think the hardcoding in this case is probably fine. We are hardcoding the input path and the expected value based on that?

Yes, based on that initialisation. I think it's fine for now, don't think it's worth to refactor it now, it might bring new issues 😄

[alexhermida]

I think those two test failing are related to the dev merge as they're currently failing in the last dev branch commit 😓

[mashehu]

yes, sorry, two tests are currently borked. you have my permission to ignore them for now and merge 😉

[alexhermida]

yes, sorry, two tests are currently borked. you have my permission to ignore them for now and merge 😉

Thanks @mashehu ! since I've merged latest dev branch commit as it looked fixed I think it needs reapproval for merging 😄

[alexhermida]

branch rebased to dev for triggering tests @mashehu thanks!

[alexhermida]

@mashehu I don't have permissions to merge 😄