Skip to content

[java] Rework Java sources discovery #303

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

Merged
merged 4 commits into from
Dec 20, 2024
Merged

[java] Rework Java sources discovery #303

merged 4 commits into from
Dec 20, 2024

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Dec 7, 2024

This is the first step – and a big one – in redoing Orchard's interaction with Java sources. It's kinda difficult to split it into smaller chunks, so I'm going to explain the changes below.

  1. The main idea is this – the user may have some downloaded Java sources on the disk. We used to look for those sources on classpath, but it's not obligatory. In fact, there is no reason to put the sources themselves on the classpath, we can read them of the disk just the same, just have to discover them.
  2. The biggest obvious place where we can find Java sources is the Java home directory. We know where it is, we know how to find the source archive in it, thus we can parse sources and enable jump-to-definition for those without having them on the classpath.
  3. Another possible source of sources (heh) are the -sources.jar JARs that can be downloaded from Maven. Getting them from Maven is kind of complicated – that's why enrich-classpath exists. But once they are downloaded, we can again locate them on the disk without having to bring them onto the classpath.
  4. A new namespace orchard.java.source-files currently contains code that sources for those source archives and JARs. It replaces the current approach of only calling io/resource to search for Java source files. In the future, this namespace might also enable the downloading of JARs somehow (e.g., by calling an external process), but it already brings certain value as is.
  5. Jumping to the source file doesn't have to obligate parsing that source file first. Sure, if we parse it, then we have the exact line number, but even without that simply opening the Java source file is already beneficial. So, this PR separates the logic of locating the source file and parsing it, so the latter is not required for the former.
  6. I've also updated other functions to play well with these changes.

I'm going to test-drive these changes for a while.

@alexander-yakushev alexander-yakushev force-pushed the java-src-discovery branch 17 times, most recently from cc1366e to 9e63466 Compare December 7, 2024 20:28
@bbatsov
Copy link
Member

bbatsov commented Dec 8, 2024

I totally agree on all points and the PR looks pretty good to me in its current state. Just a couple of small notes:

  • might be good to convert your PR summary into a README section named "Dealing with Java sources" or something like this, as it's pretty good general documentation IMO
  • I've noticed you've left some code you probably used to test the functionality while developing it (commented out forms) - I'm not sure if you meant to keep them in the final form of the PR or not; I'm fine either way

The main idea is this – the user may have some downloaded Java sources on the disk. We used to look for those sources on classpath, but it's not obligatory. In fact, there is no reason to put the sources themselves on the classpath, we can read them of the disk just the same, just have to discover them.

I guess us working in this direction originally is on me, as I thought it'd be easier to locate sources in this manner - I've learned my lesson since. 😅

(ns orchard.java.source-files
"Contains functions for discovering Java source files that are already available
on the REPL and for downloading sources from Maven."
(:require [clojure.java.io :as io]
Copy link
Member

Choose a reason for hiding this comment

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

You might also want to add the usual namespace metadata here.

@alexander-yakushev alexander-yakushev force-pushed the java-src-discovery branch 3 times, most recently from 12f1a57 to b7689b3 Compare December 9, 2024 15:23
@alexander-yakushev
Copy link
Member Author

Updated the README as you requested and added ns meta. I'm not rushing to merge this, let it sit while I'm finding the bugs in practice (there should be some).

@alexander-yakushev alexander-yakushev force-pushed the java-src-discovery branch 3 times, most recently from eb1bb62 to 2702cbb Compare December 9, 2024 21:20
@bbatsov
Copy link
Member

bbatsov commented Dec 9, 2024

Btw, it's probably still worth mentioning enrich in the context of downloading automatically the sources of 3rd party Java packages.

@alexander-yakushev
Copy link
Member Author

True, I'll put it into the new Java sources README section.

@alexander-yakushev
Copy link
Member Author

OK, I used it for some time, and I'm now ready to merge.

@bbatsov bbatsov merged commit a2bbbda into master Dec 20, 2024
19 checks passed
@bbatsov bbatsov deleted the java-src-discovery branch December 20, 2024 22:57
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.

2 participants