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

Quality-of-life improvements for edit #1211

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

quintesse
Copy link
Contributor

@quintesse quintesse commented Jan 24, 2022

In case no managed VSCodium is installed, Jbang will now search the
user's PATH for well-known editors and present them as additional
options next to offer of downloading VSCodium.
If the user selects one of the editors found on the PATH, Jbang will
show the config command the user can use to set that option as the
default for future invocations.
A new option was added to explicitely exit without doing anything.

Fixes #1210

@quintesse quintesse requested a review from maxandersen January 24, 2022 23:21
@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #1211 (5fef94e) into main (d683e0a) will decrease coverage by 0.14%.
The diff coverage is 25.53%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1211      +/-   ##
============================================
- Coverage     58.82%   58.67%   -0.15%     
- Complexity     1057     1058       +1     
============================================
  Files            86       86              
  Lines          5661     5689      +28     
  Branches        954      960       +6     
============================================
+ Hits           3330     3338       +8     
- Misses         1839     1858      +19     
- Partials        492      493       +1     
Flag Coverage Δ
Linux 57.42% <14.89%> (-0.25%) ⬇️
Windows 57.42% <21.27%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/java/dev/jbang/cli/Edit.java 46.35% <2.85%> (-4.36%) ⬇️
src/main/java/dev/jbang/util/Util.java 61.10% <90.90%> (+0.66%) ⬆️
src/main/java/dev/jbang/cli/Init.java 84.69% <100.00%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3adca1b...5fef94e. Read the comment docs.

}

private static List<String> findEditorsOnPath() {
return Arrays.stream(knownEditors).filter(e -> Util.searchPath(e) != null).collect(Collectors.toList());
Copy link
Collaborator

Choose a reason for hiding this comment

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

this won't find editors on windows where they potentially have .exe/.bat/.cmd/.ps1/etc suffixes.

would need to search with multiple suffixes.

Another option is to scan running processes to help decide which one wins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tsk tsk tsk, ye of little faith :-)

	public static Path searchPath(String name) {
		String envPath = System.getenv("PATH");
		envPath = envPath != null ? envPath : "";
		return Arrays	.stream(envPath.split(File.pathSeparator))
                    .map(Paths::get)
                    .filter(p -> isExecutable(p.resolve(name)))  <########
                    .findFirst()
                    .orElse(null);
	}

	private static boolean isExecutable(Path file) {
		if (Files.isExecutable(file)) {
			if (Util.isWindows()) {
				String nm = file.getFileName().toString().toLowerCase();
#######>		        return nm.endsWith(".exe") || nm.endsWith(".bat") || nm.endsWith(".cmd") || nm.endsWith(".ps1");
			} else {
				return true;
			}
		}
		return false;
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still, it did not actually work as advertised 😅 But it's fixed now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

:)

@quintesse quintesse marked this pull request as draft January 25, 2022 12:49
@quintesse quintesse force-pushed the ask_editors branch 2 times, most recently from a923119 to f976f53 Compare January 25, 2022 14:06
@quintesse quintesse marked this pull request as ready for review January 25, 2022 15:45
@quintesse quintesse requested a review from maxandersen January 27, 2022 22:08
In case no managed VSCodium is installed, Jbang will now search the
user's PATH for well-known editors and present them as additional
options next to offer of downloading VSCodium.
If the user selects one of the editors found on the PATH, Jbang will
show the `config` command the user can use to set that option as the
default for future invocations.
A new option was added to explicitely exit without doing anything.

Fixes jbangdev#1210
@maxandersen
Copy link
Collaborator

when I build this the tests never completes? or it at least been running for over 10 minutes now...

@maxandersen
Copy link
Collaborator

hmm - seems I nuked my maven repo and we have some big resolves ....

@maxandersen
Copy link
Collaborator

works pretty nice - i'm torn on wether vscodium should still be treated special. i.e. once you installed vscodium you wont get the question about which IDE to use...but if I choose idea/eclipse/code I'm just told to manually set the config...

@maxandersen maxandersen merged commit 5de28c2 into jbangdev:main Feb 1, 2022
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.

Make jbang edit --open smarter and search for IDEs on user's PATH
2 participants