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

Added support for consecutive asterisks at the beginning or end #20

Merged
merged 4 commits into from
Jun 29, 2022

Conversation

martinssipenko
Copy link
Contributor

According to CODEOWNERS Syntax doc, CODEOWNERS file uses a pattern that follows most of the same rules used in gitignore files.

This Pull Requests adds support for consecutive asterisks at the beginning or end (**/foo, foo/**). The case where asterisks are in the middle (a/**/b) was already supported.

@martinssipenko martinssipenko marked this pull request as ready for review June 28, 2022 08:51
@timoschinkel
Copy link
Owner

Hi Martins,

Thank you for your pull request. I will try to have a closer look later this week. On first glance it looks good, although I'm wondering about the replacement of /** with .*, maybe that should be \/.*.

I see that something has changed in the configuration of Psalm causing the inspections to fail. I will look at that as well.

* main:
  Update Psalm and remove deprecated `totallyTyped` attribute
@martinssipenko
Copy link
Contributor Author

I fixed Psalm in #21 and rebased this PR.

I just tried using \/.* instead of .* locally and the tests were failing. Honestly, I have not invested time to fully understand the inner working of the library, so I don't know what would be the best solution. I encountered an issue with **/foo case, so I added a test and patched code to make it work, duplicating the ** case (.*) seemed the simplest option.

@timoschinkel
Copy link
Owner

No problem. I will try to look at your pull request before the end of the week. This pull request requires a bit more attention compared to #21.

@martinssipenko
Copy link
Contributor Author

Sure, in the mean time I'll use my forked version. Thanks!

timoschinkel added a commit to timoschinkel/test-repository that referenced this pull request Jun 28, 2022
timoschinkel added a commit to timoschinkel/test-repository that referenced this pull request Jun 28, 2022
timoschinkel added a commit to timoschinkel/test-repository that referenced this pull request Jun 29, 2022
Copy link
Owner

@timoschinkel timoschinkel left a comment

Choose a reason for hiding this comment

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

Can you add the suggested test cases to \CodeOwners\Tests\PatternMatcherTest::provideNoMatchFoundExceptionIsThrownForFilename()? I think your current solution is too forgiving and will result in incorrect results.

@@ -62,6 +62,8 @@ private function isMatch(Pattern $pattern, string $filename): bool
'*' => '[^\/]+',
'?' => '[^\/]',
'**' => '.*',
'/**' => '.*',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
'/**' => '.*',
'/**' => '\/.*',

I think we do need to add the slashes. I have created a pull request on a test repository to verify some edge cases. I have added a coworker as codeowner to that repository: https://github.com/timoschinkel/test-repository/pull/6/files

For the rule docs/** that does apply to docs/file.md and docs/folder/file.md, but not to docs.md.

These situations can be tested by adding the following lines to \CodeOwners\Tests\PatternMatcherTest::provideNoMatchFoundExceptionIsThrownForFilename():

            // **
            [new Pattern('**/foo', ['@owner']), 'foo.ext'],
            [new Pattern('foo/**', ['@owner']), 'foo.ext'],

@martinssipenko
Copy link
Contributor Author

@timoschinkel Updated code as per your suggestions.

Copy link
Owner

@timoschinkel timoschinkel left a comment

Choose a reason for hiding this comment

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

Sorry, but I did find an issue with the **/ pattern. I've supplied a test case and I have verified this with a pull request on GitHub: https://github.com/timoschinkel/test-repository/pull/6/files

If we apply this fix then this change can be merged I think.

src/PatternMatcher.php Outdated Show resolved Hide resolved
tests/PatternMatcherTest.php Show resolved Hide resolved
@martinssipenko
Copy link
Contributor Author

@timoschinkel done

@timoschinkel timoschinkel merged commit 3d745ef into timoschinkel:main Jun 29, 2022
@timoschinkel
Copy link
Owner

Thank you for the contribution. I just released this in 2.1.0.

@martinssipenko martinssipenko deleted the extra-patters branch June 30, 2022 06:04
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.

2 participants