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

Use DirectoryPathTree in codegen #761

Merged
merged 1 commit into from
Mar 20, 2023
Merged

Conversation

scrocquesel
Copy link
Member

/cc @ppalaga

@ppalaga
Copy link
Contributor

ppalaga commented Mar 14, 2023

@aloubyansky any chance to add some JavaDoc to the classes under independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths? It is very hard to review this PR without knowing basic things about DirectoryPathTree, like are the paths Windows/Unix normalized? Are the visited paths relative? Are third party extensions even allowed to use it? Why is DirectoryPathTreeTest.walk using PathTree.ofDirectoryOrArchive(root) rather than new DirectoryPathTree(root) directly (how can I be sure that DirectoryPathTree is tested at all)?

@ppalaga
Copy link
Contributor

ppalaga commented Mar 14, 2023

Is there a regular Windows CI for DirectoryPathTreeTest?

@aloubyansky
Copy link
Member

This test is a part of the Quarkus CI, which includes Windows.
There is more javadoc in the PathTree interface that at least IDEs properly display for the implementations too. The directory-based one is one of the available implementations. Generally, I'd suggest using PathTree.ofDirectoryOrArchive(path) which would return an impl based on the path.
PathTree API can be used in other extensions, the primary purpose of this API is to provide classpath resources to the classloaders and users of the ClassLoader API.
The Paths exposed through the PathTree API will depend on the FileSystem that created them. AFAICS, if you initialize the directory-based impl yourself, it does not make sure it's an absolute path and will use it as it is.
Yes, changing the unit test to instantiate the DirectoryPathTree directly would make more sense than relying on a facade API.

@scrocquesel
Copy link
Member Author

This test is a part of the Quarkus CI, which includes Windows.
There is more javadoc in the PathTree interface that at least IDEs properly display for the implementations too. The directory-based one is one of the available implementations. Generally, I'd suggest using PathTree.ofDirectoryOrArchive(path) which would return an impl based on the path.
PathTree API can be used in other extensions, the primary purpose of this API is to provide classpath resources to the classloaders and users of the ClassLoader API.
The Paths exposed through the PathTree API will depend on the FileSystem that created them. AFAICS, if you initialize the directory-based impl yourself, it does not make sure it's an absolute path and will use it as it is.
Yes, changing the unit test to instantiate the DirectoryPathTree directly would make more sense than relying on a facade API.

@ppalaga for now, I didn't want to use the factory abstraction because wsdl2java would not be able to process file inside archive. This would need to copy files in a temp directory. I plan to do this to be able to import wsdl from imported dependencies.

@aloubyansky
Copy link
Member

@scrocquesel the tree returned from the facade API will depends on the input you provide. If you provide a directory, it'll be based on the default FileSystem. If you provide an archive, it will be a directory-based tree but based on the ZIP FileSystem impl.

@ppalaga
Copy link
Contributor

ppalaga commented Mar 20, 2023

This test is a part of the Quarkus CI, which includes Windows.

Perfect, thanks!

There is more javadoc in the PathTree interface

Thanks for pointing to that, the JavaDoc there is really useful. Having some JavaDoc on PathVisitor and PathVisit would be nice too, esp. the info which paths are relative and relative to what.

@ppalaga ppalaga merged commit e599d6b into quarkiverse:main Mar 20, 2023
@aloubyansky
Copy link
Member

I added a bit javadoc here quarkusio/quarkus#31939

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.

3 participants