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.

Sign up for GitHub

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

Jump to bottom

Update to commons-io 2.11.0 #1138

Merged
merged 4 commits into from Sep 29, 2023
Merged

Update to commons-io 2.11.0 #1138

merged 4 commits into from Sep 29, 2023

Conversation

guillaume-alvarez
Copy link
Contributor

Fix #1136

ppodgorsek
ppodgorsek requested changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@ppodgorsek ppodgorsek left a comment

Choose a reason for hiding this comment

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

The CI pipeline failed after the PR was upgraded to the latest version from master.
Could you fix the issues please?

@guillaume-alvarez
Copy link
Contributor Author

The CI pipeline failed after the PR was upgraded to the latest version from master. Could you fix the issues please?

I rebased on latest master and fixed the issue.

@ppodgorsek
Copy link
Contributor

The CI pipeline failed after the PR was upgraded to the latest version from master. Could you fix the issues please?

I rebased on latest master and fixed the issue.

Thank you @guillaume-alvarez, I didn't have as much time as I hoped but I will definitely review it next week. Have a nice weekend! :)

ppodgorsek
ppodgorsek requested changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@ppodgorsek ppodgorsek left a comment

Choose a reason for hiding this comment

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

Thank you Guillaume, I finally reviewed this PR and had more time to research this issue with commons IO. It feels like those changes are related to a regression in the commons-io library: https://issues.apache.org/jira/browse/IO-754

I think we should follow the official recommendation and rely on accept(Path path, BasicFileAttributes attributes) instead, to keep our logic in line with how the library has evolved.

Would you mind doing those changes please?

.../src/main/java/ro/isdc/wro/model/resource/locator/wildcard/DefaultWildcardStreamLocator.java Outdated Show resolved Hide resolved
@kevinleturc
Copy link

Hello @ppodgorsek,
Apache Commons IO has deprecated the constructors of WildcardFileFilter in favor of a builder pattern, see https://github.com/apache/commons-io/blob/master/src/main/java/org/apache/commons/io/filefilter/WildcardFileFilter.java#L183.
Keeping the anonymous declaration will break at future commons-io upgrade.

Do you want to keep it and change the accept method to override?

@kevinleturc kevinleturc mentioned this pull request Aug 11, 2023
Closed
@ppodgorsek
Copy link
Contributor

Hello @kevinleturc ,

That's a very good point! Thank you for the information :)
It definitely makes sense to follow their longer-term approach if the constructors have been deprecated.

@ppodgorsek ppodgorsek added the dependencies Pull requests that update a dependency file label Sep 28, 2023
@ppodgorsek ppodgorsek added this to In progress in Wro4j 2.1.0 via automation Sep 28, 2023
@ppodgorsek ppodgorsek added this to the 2.0.1 milestone Sep 28, 2023
@ppodgorsek
Copy link
Contributor

A few tweaks have been made to the PR. Additionally, the builder pattern mentioned by @kevinleturc doesn't yet exist in commons-io 2.11.0, so a separate ticket (#1159) has been created to take care of this. (commons-io 2.13.0 introduces many other changes which break many tests, hence why it will be handled in a separate ticket)

@ppodgorsek ppodgorsek merged commit 6214dc2 into wro4j:master Sep 29, 2023
Wro4j 2.1.0 automation moved this from In progress to Done Sep 29, 2023
@ppodgorsek
Copy link
Contributor

Thanks @guillaume-alvarez for your contribution! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppodgorsek ppodgorsek ppodgorsek approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
No open projects
Wro4j 2.1.0
Done
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.

Upgrade commons-io from 2.8.0 to 2.11.0
3 participants
@guillaume-alvarez @ppodgorsek @kevinleturc