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

Implement dot option #316

Merged
merged 29 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
b0d9292
Implement dot option
kachkaev Feb 4, 2022
6a0b726
Rebuild
kachkaev Feb 4, 2022
409a5a2
Fix typo in test caption
kachkaev Feb 4, 2022
920e9b6
Add comments
kachkaev Feb 4, 2022
8b8677b
Improve warning
kachkaev Feb 4, 2022
e05b7c1
Update README.md
kachkaev Feb 4, 2022
f3632d3
Update README.md
kachkaev Feb 4, 2022
d89797c
Merge remote-tracking branch 'upstream/main' into dot-option
kachkaev Sep 14, 2022
2025154
Simplify glob
kachkaev Oct 5, 2022
866eff5
Merge remote-tracking branch 'origin/main' into dot-option
kachkaev Jan 16, 2023
305cfeb
Rebuild
kachkaev Jan 16, 2023
4a96e77
Merge branch 'main' into dot-option
kachkaev Mar 12, 2023
b898cc8
Merge remote-tracking branch 'u/main' into dot-option
kachkaev Mar 27, 2023
b28379f
Cleanup
kachkaev Mar 27, 2023
a7cc8a6
Fix
kachkaev Mar 27, 2023
012b892
Merge remote-tracking branch 'u/main' into dot-option
kachkaev May 19, 2023
cecbd94
Merge remote-tracking branch 'u/main' into dot-option
kachkaev May 31, 2023
092c979
Update src/labeler.ts
kachkaev May 31, 2023
60f44e7
Fix unrelated test
kachkaev May 31, 2023
9776203
Update build
kachkaev May 31, 2023
a27020c
Undo unwanted change in diff
kachkaev May 31, 2023
44414db
Fix tests
kachkaev May 31, 2023
673c7e2
Rebuild
kachkaev May 31, 2023
e3b3815
Further diff reduction
kachkaev May 31, 2023
54d434d
Remove `required: false`
kachkaev May 31, 2023
59d3310
Rebuild
kachkaev May 31, 2023
71d2484
Address review comment
kachkaev May 31, 2023
639ba81
Rebuild
kachkaev May 31, 2023
d40596e
micromatch → minimatch
kachkaev Jun 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ repo:

# Add '@domain/core' label to any change within the 'core' package
@domain/core:
- package/core/*
kachkaev marked this conversation as resolved.
Show resolved Hide resolved
- package/core/**/*

# Add 'test' label to any change to *.spec.js files within the source dir
Expand Down Expand Up @@ -113,7 +112,23 @@ Various inputs are defined in [`action.yml`](action.yml) to let you configure th
| `repo-token` | Token to use to authorize label changes. Typically the GITHUB_TOKEN secret | N/A |
| `configuration-path` | The path to the label configuration file | `.github/labeler.yml` |
| `sync-labels` | Whether or not to remove labels when matching files are reverted or no longer changed by the PR | `false`
| `dot` | Whether or not to auto-include paths starting with dot (e.g. `.github`) | `false`

Choose a reason for hiding this comment

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

WDYT about dropping the configuration option and making dot always true.

If someone really wants to ignore dot files, that is another glob expression, right? (Something already supported.) IMO, if we can solve the same problem with less configuration it will be more user friendly and more likely to "do what I mean".

Copy link
Contributor Author

@kachkaev kachkaev Sep 15, 2022

Choose a reason for hiding this comment

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

That would be a breaking change which could upset existing users. It’s safer to introduce the option, then wait for a few months, change the default in a major release, wait again and finally remove the option completely.

Copy link

@jdufresne jdufresne Sep 15, 2022

Choose a reason for hiding this comment

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

Yes, I agree it changes the behavior, but perhaps it is a reasonable one. We could also bump the major version to play it safe with expectations. Ultimately, it is not my decision. It is only a suggestion as an end user.


# Contributions
When `dot` is disabled and you want to include _all_ files in a folder:

```yml
label1:
- path/to/folder/**/*
- path/to/folder/**/.*
Comment on lines +123 to +124
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about path/to/folder/sub/.folder/hello.txt. Will it match? 🤔 If not, what would be the right set of globs?

```

If `dot` is enabled:

```yml
label1:
- path/to/folder/**/*
```

## Contributions

Contributions are welcome! See the [Contributor's Guide](CONTRIBUTING.md).
18 changes: 16 additions & 2 deletions __tests__/labeler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,29 @@ const matchConfig = [{ any: ["*.txt"] }];
describe("checkGlobs", () => {
it("returns true when our pattern does match changed files", () => {
const changedFiles = ["foo.txt", "bar.txt"];
const result = checkGlobs(changedFiles, matchConfig);
const result = checkGlobs(changedFiles, matchConfig, false);

expect(result).toBeTruthy();
});

it("returns false when our pattern does not match changed files", () => {
const changedFiles = ["foo.docx"];
const result = checkGlobs(changedFiles, matchConfig);
const result = checkGlobs(changedFiles, matchConfig, false);

expect(result).toBeFalsy();
});

it("returns false for a file starting with dot if `dot` option is false", () => {
const changedFiles = [".foo.txt"];
const result = checkGlobs(changedFiles, matchConfig, false);

expect(result).toBeFalsy();
});

it("returns false for a file starting with dot if `dot` option is true", () => {
const changedFiles = [".foo.txt"];
const result = checkGlobs(changedFiles, matchConfig, true);

expect(result).toBeTruthy();
});
});
63 changes: 49 additions & 14 deletions __tests__/main.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,24 @@ const yamlFixtures = {
"only_pdfs.yml": fs.readFileSync("__tests__/fixtures/only_pdfs.yml"),
};

const configureInput = (
mockInput: Partial<{
"repo-token": string;
"configuration-path": string;
"sync-labels": boolean;
dot: boolean;
}>
) => {
jest
.spyOn(core, "getInput")
.mockImplementation((name: string, ...opts) => mockInput[name]);
};

afterAll(() => jest.restoreAllMocks());

describe("run", () => {
it("adds labels to PRs that match our glob patterns", async () => {
it("(with dot: false) adds labels to PRs that match our glob patterns", async () => {
configureInput({});
usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles("foo.pdf");

Expand All @@ -37,7 +51,36 @@ describe("run", () => {
});
});

it("does not add labels to PRs that do not match our glob patterns", async () => {
it("(with dot: true) adds labels to PRs that match our glob patterns", async () => {
configureInput({ dot: true });
usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles(".foo.pdf");

await run();

expect(removeLabelMock).toHaveBeenCalledTimes(0);
expect(addLabelsMock).toHaveBeenCalledTimes(1);
expect(addLabelsMock).toHaveBeenCalledWith({
owner: "monalisa",
repo: "helloworld",
issue_number: 123,
labels: ["touched-a-pdf-file"],
});
});

it("(with dot: false) does not add labels to PRs that do not match our glob patterns", async () => {
configureInput({});
usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles(".foo.pdf");

await run();

expect(removeLabelMock).toHaveBeenCalledTimes(0);
expect(addLabelsMock).toHaveBeenCalledTimes(0);
});

it("(with dot: true) does not add labels to PRs that do not match our glob patterns", async () => {
configureInput({ dot: true });
usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles("foo.txt");

Expand All @@ -48,15 +91,11 @@ describe("run", () => {
});

it("(with sync-labels: true) it deletes preexisting PR labels that no longer match the glob pattern", async () => {
let mockInput = {
configureInput({
"repo-token": "foo",
"configuration-path": "bar",
"sync-labels": true,
};

jest
.spyOn(core, "getInput")
.mockImplementation((name: string, ...opts) => mockInput[name]);
});

usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles("foo.txt");
Expand All @@ -79,15 +118,11 @@ describe("run", () => {
});

it("(with sync-labels: false) it issues no delete calls even when there are preexisting PR labels that no longer match the glob pattern", async () => {
let mockInput = {
configureInput({
"repo-token": "foo",
"configuration-path": "bar",
"sync-labels": false,
};

jest
.spyOn(core, "getInput")
.mockImplementation((name: string, ...opts) => mockInput[name]);
});

usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles("foo.txt");
Expand Down
4 changes: 4 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ inputs:
description: 'Whether or not to remove labels when matching files are reverted'
default: false
required: false
dot:
description: 'Whether or not to auto-include paths starting with dot (e.g. `.github`)'
default: false
required: false

runs:
using: 'node12'
Expand Down
Loading