#2103: Integrate Obsidian - #2380
Conversation
…/2103-obsidian-commandlet
Coverage Report for CI Build 33412504001Coverage decreased (-0.008%) to 73.605%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions9 previously-covered lines in 3 files lost coverage.
Coverage Stats💛 - Coveralls |
Ali-Shariati-Najafabadi
left a comment
There was a problem hiding this comment.
Compiles, tests pass, help texts look good in both languages. Left two comments below I'd want addressed before merging.
| // the Windows download is an installer executable that has to be started, not extracted | ||
| case WINDOWS -> false; | ||
| // the macOS download is a DMG image that has to be mounted so the *.app can be taken out of it | ||
| case MAC -> true; |
There was a problem hiding this comment.
Not sure this actually installs anything on mac/linux. doInstall() extracts to a tmp dir, runs the first executable it finds, then deletes the tmp dir right after. For the mac DMG case that just opens the app once and wipes it. Docker handles this by overriding getInstalledVersion() with a check against /Applications/Docker.app — Obsidian doesn't, so it'll probably never register as installed on mac. Given ide-urls has no obsidian entries yet, this is untested — worth a real check once that's in place.
There was a problem hiding this comment.
You're right, on mac/linux doInstall() just runs the first executable it finds and then deletes the tmp dir, so nothing gets installed. Also ide-urls has obsidian now and linux_x64 is a .deb, not a .tar.gz.
I've only tested on Windows. Can we keep this PR Windows-only and open a follow-up for mac/linux?
I mean open a separate issue for that?
@hohwille
…t DOCUMENTATION tag
This PR fixes #2103
Implemented changes:
Obsidiancommandlet extendingGlobalToolCommandlet, based on the URL Updater from Create ObsidianUrlUpdater #2186 (#2186: create obsidian url updater #2203).exeis executed and not extracted.dmgis extracted so the*.appcan be taken out of it.tar.gzis extractedCommandletManagerImpldocumentation/LICENSE.adocObsidianTestcovering registration, tags and the per-OSisExtract()branchesTesting instructions
mvn clean test.ObsidianTestandHelpCommandletTestshould pass.ide help obsidianandide --locale=de help obsidian. The English and German help texts should be shown.ide install obsidianon your platform and check that Obsidian is installed.Note on step 3:
ide-urlscurrently contains no metadata for obsidian, so version resolutionfails and the installation cannot be tested yet. The metadata is only created once the URL
Updater from #2186 has run.
Checklist for this PR
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»In Progressand assigned to youpom.xmlfiles or otherwise if runtime dependencies changed, you have updated LICENSE.asciidocChecklist for tool commandlets
obsidianOBSIDIAN_VERSIONandOBSIDIAN_EDITIONare honored by your commandlet