Skip to content

Fix ignored paths check#55

Merged
pascalgn merged 3 commits intopascalgn:mainfrom
mat3e:main
Oct 23, 2024
Merged

Fix ignored paths check#55
pascalgn merged 3 commits intopascalgn:mainfrom
mat3e:main

Conversation

@mat3e
Copy link
Copy Markdown
Contributor

@mat3e mat3e commented Oct 4, 2024

Seems since using files API in #54, IGNORED stopped to work. I added tests to confirm and it seems path.slice(2) inside isIgnored is not needed anymore as the Diff adding extra chars is not used.

Seems since using files API in pascalgn#54, `IGNORED` stopped to work
@mat3e
Copy link
Copy Markdown
Contributor Author

mat3e commented Oct 4, 2024

@pascalgn, @levsa 👀

Copy link
Copy Markdown
Contributor

@levsa levsa left a comment

Choose a reason for hiding this comment

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

Good work! But I think undefined may be an issue.

// then
expect(isIgnored("file")).toBe(false);
// always ignored
expect(isIgnored(null)).toBe(true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think file.previous_filename may be undefined diffs that don't rename files? Can you add a test for isIgnored(undefined) and handle it in isIgnored()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a test and luckily it works as path is compared with null using ==, not === in index.js, line 148 👍🏻 😄

if (path == null || path === "/dev/null") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great :)

Copy link
Copy Markdown
Contributor

@levsa levsa left a comment

Choose a reason for hiding this comment

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

👍

@DaniFoldi
Copy link
Copy Markdown

Hey! I also ran into this recently after updating from 0.5.0, thanks for implementing!

@mat3e
Copy link
Copy Markdown
Contributor Author

mat3e commented Oct 22, 2024

Hey! I also ran into this recently after updating from 0.5.0, thanks for implementing!

I temporarily use my fork directly in the repo and you can try as well: mat3e/size-label-action@main 👍🏻

@pascalgn pascalgn merged commit a35c4e5 into pascalgn:main Oct 23, 2024
@pascalgn
Copy link
Copy Markdown
Owner

Thanks for the PR, it's merged now! 👍

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.

4 participants