gerlowskija commented on pull request #1600: URL: https://github.com/apache/lucene-solr/pull/1600#issuecomment-648114057
I don't mind this PR at all, I just have some questions. I don't really understand the aversion to `SolrPaths` and `File`. More detailed questions below. Treat this comment as a "+1", as long as you get back to me on the questions below. > CoreContainer seems to me the proper place for the node to expose key directories at a node level. It's already doing this. It looked a bit clumsy to me to have the String constant and extra methods elsewhere for userfiles. I don't have a strong opinion on whether this logic is invoked by SolrDispatchFilter or CoreContainer, but I'm surprised at your objection about where the logic lives. Why is `SolrPaths` a bad place for constants/logic around locating/creating elements of Solr's directory structure? AFAIK this is exactly what `SolrPaths` is for. Why clutter up CoreContainer with path logic when we have classes specifically meant to hold that? > Path is the modern replacement, not File. I wish we could do forbiddenAPI on File but there's lots of old code. If true, this is a good argument for the work Erick's doing to strengthen our compile-time enforcement of these opinions. I have no objection to `Path`, but I also had no idea that there was anything wrong with using `File`. In the diff here, the APIs look relatively identical. What reason is there to prefer Path? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
