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

Make the taskfile in include required #1902

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

semihbkgr
Copy link

fixes #1881

This makes the taskfile field within include required, disallowing null (unspecified) or empty values, even if inlined.

I’d like to clarify two points, @pd93:

1- Would it be preferable to return a TaskfileDecodeError in the UnmarshalYAML function when parsing the yaml into Include? This approach could make the error message more descriptive.

Screenshot from 2024-11-02 22-51-17

However, I aligned this error with the existing one we return when the Taskfile is not existing ("no such file or directory"), which is why the error is returned after the parsing step.

2- Should we differentiate between null and empty string values, or should we treat them the same and return an error for both?

@pd93
Copy link
Member

pd93 commented Nov 4, 2024

1- Would it be preferable to return a TaskfileDecodeError in the UnmarshalYAML function when parsing the yaml into Include? This approach could make the error message more descriptive.

However, I aligned this error with the existing one we return when the Taskfile is not existing ("no such file or directory"), which is why the error is returned after the parsing step.

The distinction here is whether or not taskfile: is required in the schema or whether it is valid to be empty in the schema, but causes an error when processing.

Opinion: I think we should error in the UnmarshalYAML method and return a TaskfileDecodeError and we should update the schema.json to mark taskfile: as a required field when using an include: object. I'm not sure if jsonschema can specify non-empty on a scalar value? but if it can, we should do that too. @andreynering @vmaerten feel free to weigh-in.

2- Should we differentiate between null and empty string values, or should we treat them the same and return an error for both?

I can't see a situation where an empty string is useful.

@semihbkgr
Copy link
Author

@pd93 We can use "minLength": 1 in the json schema to ensure the taskfile value is not empty.

@vmaerten
Copy link
Member

vmaerten commented Dec 7, 2024

I totally agree with you @pd93 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect/confusing include cycle detected error message after a silly mistake
3 participants