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

Replace usage of jruby DirectoryWalker with Java NIO API (#532) #535

Merged
merged 4 commits into from
Jul 8, 2021
Merged

Conversation

stdll
Copy link
Contributor

@stdll stdll commented Jul 7, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Documentation
  • Refactor
  • Build improvement
  • Other (please describe)

What is the goal of this pull request?
Closes #532 by replacing the class in question with the Java NIO API

Are there any alternative ways to implement this?
There are lots of 3rd party libraries to do similar stuff, but I didn't want to rely on those in favor of Java's own NIO API.

Are there any implications of this pull request? Anything a user must know?
I'm not sure about the error handling at SourceDocumentFinder (line 70). The DirectoryWalker didn't throw an exception and I didn't want to introduce one, so I handled it that way. I don't know if this is what you'd expect. If you have any feedback on this, I'm happy to change it.

Is it related to an existing issue?

  • Yes
  • No

Fixes #532

CHANGELOG.adoc Outdated Show resolved Hide resolved
@abelsromero
Copy link
Member

Thanks a lot for the PR, keeping this things is important! The only important thing is the sourceDocumentExtensions leaky pattern. Feel free to create a dedicated set of folders and files for it if you need to.

stdll and others added 2 commits July 8, 2021 09:25
Co-authored-by: Abel Salgado Romero <abelromero@gmail.com>
@stdll
Copy link
Contributor Author

stdll commented Jul 8, 2021

I've updated the PR. I've integrated your solution to joining, added a few tests for file extensions and clarified the changelog entry.

@abelsromero
Copy link
Member

Great! Thanks a lot!

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.

Remove usage of internal Asciidoctorj DirectoryWalker
2 participants