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

feat(fingreprinting): add .taskignore #1932

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GeneTakavic24
Copy link

Fixes #1753

@GeneTakavic24
Copy link
Author

GeneTakavic24 commented Nov 27, 2024

Greetings, and thanks for a wonderful project!

The reasoning behind this PR is as follows:

Design

Since the project already heavily relies on glob semantics, aligning the same semantics in the Taskfile's exclude: section and .taskignore can provide the following benefits:

  1. Unified Pattern Usage: End users can use patterns interchangeably across the exclude: section and .taskignore.
  • The exclude: section currently does not support the same negation pattern semantics as .gitignore.

  • If .taskignore strictly follows .gitignore semantics, it might cause confusion and a disjointed interface for users needing consistent formatting across both.

  • On the other hand, modifying exclude to follow .gitignore semantics would introduce breaking changes, as exclude no longer adheres to the glob pattern syntax.

  1. Change scope is smaller
  • Parsing .gitignore even with the usage of a library such as lib provided might need to change the Glob structure:
    as well as fingerprinting functionality to combine both exisiting glob parsing and .gitignore-like parsing.
    type Glob struct {
    Glob string
    Negate bool
    }

@andreynering andreynering self-requested a review November 27, 2024 13:48
@GeneTakavic24 GeneTakavic24 marked this pull request as ready for review November 27, 2024 14:56
@@ -789,6 +789,37 @@ tasks:
- public/bundle.css
```

It is also possible to exclude files from fingerprinting globally by creating `.taskignore`

Choose a reason for hiding this comment

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

nit: I would add "a" -- that is, change

[...] creating .taskignore file [...]

to

[...] creating a .taskignore file [...]

@@ -0,0 +1 @@
./**/*.json

Choose a reason for hiding this comment

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

Suggest adding a comment line to make sure comments are correctly processed:

# ./dont_ignore.txt

Choose a reason for hiding this comment

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

Also some blank lines to make sure trimming also works correctly.


for _, line := range lines {
trimmed := strings.TrimSpace(line)
if trimmed != "" && !strings.HasPrefix(trimmed, "#") {

Choose a reason for hiding this comment

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

Users might expect that end-line comments (e.g., ./**/*.json # ignore pesky JSON files) are allowed, as they tend to be allowed in similar file formats.

Copy link

@adamdicarlo0 adamdicarlo0 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me.

If exclude supports negations, I would clarify in the docs and code whether negations are allowed in the .taskignore file (and unit test it).

I'm not sure whether they'd be supported, from looking at the code (I don't recall whether exclude supports them).

NB: I am not a maintainer or (at least not yet) even a contributor to the project!

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.

Make sources: respect a new .taskignore file, similar to .gitignore
2 participants