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

Fix pattern detection in relative path condition (#89) #106

Merged
merged 7 commits into from
Oct 20, 2023

Conversation

ludovic-pourrat
Copy link
Contributor

Error in IDE "Failed to check if current file is included" #89

Error in IDE "Failed to check if current file is included" SchwarzIT#89
@markbrockhoff
Copy link
Member

Hi, thanks for your contribution.

Your code looks good but could you please add a test case for the problem it fixes? Maybe just reuse the failing parameters reported inside the issue. There's already a data driven test for the isFileIncluded function.

In order to release it, please also add a "Fixes" entry to the file CHANGELOG.md under the [Unreleased] section.
This project uses KeepAChangelog for automatic versioning. In order to trigger a release you also need to bump the pluginVersion inside the gradle.properties from 3.0.0 to 3.0.1.

@tintin92350
Copy link

Hi, thanks for your contribution.

Your code looks good but could you please add a test case for the problem it fixes? Maybe just reuse the failing parameters reported inside the issue. There's already a data driven test for the isFileIncluded function.

In order to release it, please also add a "Fixes" entry to the file CHANGELOG.md under the [Unreleased] section. This project uses KeepAChangelog for automatic versioning. In order to trigger a release you also need to bump the pluginVersion inside the gradle.properties from 3.0.0 to 3.0.1.

Hi ! Well this code fix the existing unit tests that fails because of pattern like **/*.yaml that can be set by users. So I think that we don't have to create another tests as they already exists ? 😊

@markbrockhoff
Copy link
Member

Hi, thanks for your contribution.
Your code looks good but could you please add a test case for the problem it fixes? Maybe just reuse the failing parameters reported inside the issue. There's already a data driven test for the isFileIncluded function.
In order to release it, please also add a "Fixes" entry to the file CHANGELOG.md under the [Unreleased] section. This project uses KeepAChangelog for automatic versioning. In order to trigger a release you also need to bump the pluginVersion inside the gradle.properties from 3.0.0 to 3.0.1.

Hi ! Well this code fix the existing unit tests that fails because of pattern like **/*.yaml that can be set by users. So I think that we don't have to create another tests as they already exists ? 😊

I see, could it be that you're working on windows?
Because on mac and linux (our CI) all tests were already passing beforehand.

@tintin92350
Copy link

Hi, thanks for your contribution.
Your code looks good but could you please add a test case for the problem it fixes? Maybe just reuse the failing parameters reported inside the issue. There's already a data driven test for the isFileIncluded function.
In order to release it, please also add a "Fixes" entry to the file CHANGELOG.md under the [Unreleased] section. This project uses KeepAChangelog for automatic versioning. In order to trigger a release you also need to bump the pluginVersion inside the gradle.properties from 3.0.0 to 3.0.1.

Hi ! Well this code fix the existing unit tests that fails because of pattern like **/*.yaml that can be set by users. So I think that we don't have to create another tests as they already exists ? 😊

I see, could it be that you're working on windows? Because on mac and linux (our CI) all tests were already passing beforehand.

Yes ! We are working on windows environment. Mmh maybe do you want us to add an os detection condition to make the code works a little bit differently on windows ?

@markbrockhoff
Copy link
Member

markbrockhoff commented Oct 18, 2023

Hi, thanks for your contribution.
Your code looks good but could you please add a test case for the problem it fixes? Maybe just reuse the failing parameters reported inside the issue. There's already a data driven test for the isFileIncluded function.
In order to release it, please also add a "Fixes" entry to the file CHANGELOG.md under the [Unreleased] section. This project uses KeepAChangelog for automatic versioning. In order to trigger a release you also need to bump the pluginVersion inside the gradle.properties from 3.0.0 to 3.0.1.

Hi ! Well this code fix the existing unit tests that fails because of pattern like **/*.yaml that can be set by users. So I think that we don't have to create another tests as they already exists ? 😊

I see, could it be that you're working on windows? Because on mac and linux (our CI) all tests were already passing beforehand.

Yes ! We are working on windows environment. Mmh maybe do you want us to add an os detection condition to make the code works a little bit differently on windows ?

No, it think we can make it work with the same logic for windows and unix. Maintaining two sets of logic is just more effort and might introduce bugs, I'd like to avoid that if possible.
I just created the branch run-ci-on-windows inside this repo. @ludovic-pourrat Could you please rebase your branch on that so we can properly verify that all tests are passing on windows as well as on unix systems?

@ludovic-pourrat
Copy link
Contributor Author

This might be ok now for this PR, let us know.

Thanks !

@markbrockhoff
Copy link
Member

Hi @ludovic-pourrat, this looks good. Thanks again for your contribution :)
I'll merge and release this, please keep in mind that it can take up to a few days until the new release was reviewed and approved by Jetbrains and is visible in the marketplace.

@markbrockhoff markbrockhoff merged commit 12da380 into SchwarzIT:main Oct 20, 2023
4 checks passed
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.

None yet

3 participants