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

refactor: implement task list filtering #890

Merged
merged 1 commit into from
Nov 2, 2022
Merged

Conversation

pd93
Copy link
Member

@pd93 pd93 commented Oct 10, 2022

This PR refactors the code used to fetch the list of tasks when outputting them to the CLI. This is used for the --list and --list-all flags as well as when any errors are displayed to the user.

Why?

This code has a lot of duplication in it and isn't very extensible. i.e. it requires a lot of custom code and bloat to add new features.

Shiny things

I've added a GetTaskList function to the executor (this nicely matches the new GetTask function in #879). This function will return a sorted list of tasks from the task map and filter any tasks according to the list of given filters. These filters can be any function that matches the new FilterFunc type.

I have implemented the two filter functions that are currently required in the codebase:

  • FilterNoDesc - Removes tasks with no description
  • FilterInternal - Removes tasks that are marked as internal

To add a new filter, all you'd need to do is create a function that takes a single task, decides if it should be filtered or not and return true/false accordingly. This function should then be wrapped with the Filter helper function which does all the actual filtering for you.

Note: The Filter function uses an in-place, zero-allocation method for filtering. This is much more efficient than creating a new slice for each filter and using append many times. See this StackOverflow post for details.

Sorting

This PR also happens to fix #806. The duplication of the task list sorting was what led me down this path in the first place. The sort function has been replaced with something that will put tasks without a : first.

It's worth noting that #806 requested that the tasks in the root Taskfile get sorted at the top of the list. This will not necessarily happen as if you create a task called foo:bar in your root Taskfile, it will see it as a namespace. However, I think this behaviour is fine (good, even).

Other

Worth noting that the error handling for there being no tasks has moved from help.go into the cmd/task.go file. This is because functions aren't comparable and therefore ListTasks can't check if the FilterNoDesc function was given or not and decide which error to show.

This has the knock-on effect that the task: No tasks with description available. Try --list-all to list all tasks error message will no longer display when an error occurs and there are no tasks with descriptions. I personally don't mind this as it felt like a side-effect anyway, but I wanted to gather thoughts from others.

Copy link
Member

@andreynering andreynering left a comment

Choose a reason for hiding this comment

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

Looks good! 👏

I'd consider renaming FilterInternal and FilterNoDesc, though, because the current name looks like we're filtering only with desc or only internal instead of filtering out them.

Could be something like FilterOutInternal or RejectInternal (ideas, no strong opinion).

Once addressed, you can merge this PR yourself 🙂. A changelog entry mentioned the sorting of : first is welcome as well.

@pd93 pd93 force-pushed the task-list-filtering branch from 7c3aba0 to 8c328e0 Compare November 2, 2022 14:38
@pd93 pd93 force-pushed the task-list-filtering branch from 8c328e0 to fa105a8 Compare November 2, 2022 14:43
@pd93 pd93 merged commit a664a26 into master Nov 2, 2022
@pd93 pd93 deleted the task-list-filtering branch November 2, 2022 14:49
@pd93 pd93 added type: feature A new feature or functionality. and removed type: enhancement labels Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature or functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On --list, show tasks in the root Taskfile first
2 participants