Skip to content

Add contribution guidelines for Taskfiles.#14

Merged
kirkrodrigues merged 5 commits into
y-scope:mainfrom
kirkrodrigues:taskfile-guidelines
May 27, 2024
Merged

Add contribution guidelines for Taskfiles.#14
kirkrodrigues merged 5 commits into
y-scope:mainfrom
kirkrodrigues:taskfile-guidelines

Conversation

@kirkrodrigues
Copy link
Copy Markdown
Member

Description

This PR starts the contribution guidelines section and open-sources our guidelines for Taskfiles.

Validation performed

  • task serve
  • Validated the new docs look correct.

Comment thread docs/dev-guide/contrib-guides-taskfiles.md Outdated
```yaml
my-task:
cmds:
- "touch '{{.FILE_PATH}}'"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume we don't mention "strings should be quoted" in the first place because "have automated linting processes", right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep

Taskfile), replace the `:` in the name with `#` as follows: `{{.TASK | replace \":\" \"#\"}}`.
* This ensures the task name can be used as a filename.

## `sources` and `generates`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to mention how (and why) we want to checksum certain tasks instead of this sources + generates pattern?
If we're short of time, we can do that in another PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. Added.

* `cmds`
* `ignore_error`
* `interactive`
* Logging
Copy link
Copy Markdown
Member

@junhaoliao junhaoliao May 26, 2024

Choose a reason for hiding this comment

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

I feel it might be more intuitive for readers if we place these next to Descriptors.

@@ -0,0 +1,161 @@
# Taskfiles
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Although not Taskfile-specific, do we want to mention in YAML how we place array elements into separate lines where they can't fit into one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. I would suggest we do it in another PR when we add YAML guidelines, and then add a link to them from the Taskfile guidelines.


## Ordering of list/map entries

List/map entries should be ordered alphabetically (except for `vars`). Where there are several
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to mention the alphabetical order is case insensitive? The leading characters might be in different cases in paths specified in sources and generates for example.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

True, added.

@kirkrodrigues kirkrodrigues requested a review from junhaoliao May 27, 2024 02:35
Copy link
Copy Markdown
Member

@junhaoliao junhaoliao left a comment

Choose a reason for hiding this comment

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

The PR title is fine for the squashed commit message.

@kirkrodrigues kirkrodrigues merged commit 91edc4b into y-scope:main May 27, 2024
@kirkrodrigues kirkrodrigues deleted the taskfile-guidelines branch May 27, 2024 08:40
Comment on lines +40 to +42
1. Don't use the variable `{{.ROOT_TASKFILE}}`
* v3.35.1 has a bug that makes it equivalent to `ROOT_DIR` (i.e., the parent directory of the root
Taskfile, rather than the path to the root Taskfile).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be removed now given these days our projects use a newer version of go-task which doesn't have this bug

@coderabbitai create an issue to track

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

gh issue create \
  --repo y-scope/yscope-docs \
  --title 'Remove outdated `{{.ROOT_TASKFILE}}` bug note from Taskfiles contrib guide' \
  --body '## Summary

The Taskfiles contribution guide (`docs/dev-guide/contrib-guides-taskfiles.md`) contains a note warning against using `{{.ROOT_TASKFILE}}` due to a bug in go-task v3.35.1 that caused it to behave like `ROOT_DIR`.

Since our projects now use a newer version of go-task in which this bug has been fixed, this note should be removed.

## Action required

- Remove the `{{.ROOT_TASKFILE}}` built-in variable warning note from `docs/dev-guide/contrib-guides-taskfiles.md`.

## References

- PR: https://github.com/y-scope/yscope-docs/pull/14
- Comment: https://github.com/y-scope/yscope-docs/pull/14#discussion_r2983398850

Requested by: `@junhaoliao`' \
  --assignee junhaoliao

Length of output: 48


@junhaoliao, I've created issue #57 to track the removal of the outdated {{.ROOT_TASKFILE}} bug note: #57

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.

2 participants