feat: remove logger from taskfile package #2082
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pd93
changed the title
vmaerten
reviewed
Feb 22, 2025
| @@ -117,7 +115,6 @@ func Exists(l *logger.Logger, path string) (string, error) { | |||
| for _, taskfile := range defaultTaskfiles { | |||
| alt := filepathext.SmartJoin(path, taskfile) | |||
| if _, err := os.Stat(alt); err == nil { | |||
| l.VerboseOutf(logger.Magenta, "task: [%s] Not found - Using alternative (%s)\n", path, taskfile) | |||
There was a problem hiding this comment.
I’m OK with removing it because, even though we could use it for troubleshooting, I don’t think it’s used at all. I just wanted to make sure it wasn’t an oversight.
There was a problem hiding this comment.
Yeah, its difficult to log here without passing a function in. If we really want the logs, we could compare the input/output in the caller and log there instead, but I decided it wasn't worth it.
pd93
added a commit
that referenced
this pull request
Feb 22, 2025
pd93
added
the
area: package api
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our package API should not rely on our internal
loggerpackage. Logging is a CLI-specific responsibility and users that want to use Task as a package may want to implement their own logging. As such, this PR removes the logger from thetaskfilepackage entirely.Normally this would mean we lose the ability to insert debug logs into our code. However, we can pass a generic function into the
Readerwhich will process logs instead. This allows all of Task's current logging to remain in place while a 3rd party user can either omit the logging function when creating their reader (and discard the logs) or implement their own custom logging.I have also done the same thing for the prompt function so that package API users can implement their own prompt functionality.
Finally, the reader now uses functional options in its constructor. This allows us to set sensible defaults instead of the caller having to specify everything each time (including the new logging functions).