search

coddingtonbear / obsidian-local-rest-api

Unlock your automation needs by interacting with your notes in Obsidian over a secure REST API.
MIT License
607 stars 56 forks source link

feat: auto create dirs #44

Closed SettingDust closed 1 year ago

SettingDust commented 1 year ago

fix #42

coddingtonbear commented 1 year ago

@SettingDust -- I'm at the moment assuming you've run out of time to work on this pull request; so I'm going to close this request due to inactivity. If you did find the time to pick this up again, though, feel free to re-open or ask me to once you've worked through the directory creation issues discussed above.

SettingDust commented 1 year ago

Sry. I forgot this PR. I've fixed this issue by using fs.mkdir. Have pushed

SettingDust commented 1 year ago

Due to https://discord.com/channels/686053708261228577/840286264964022302/1104023861449728060

Obsidian 1.2.7 should fix this issue. But I need modify something by the guideline. https://docs.obsidian.md/Plugins/Releasing/Plugin+guidelines#Prefer+the+Vault+API+over+the+Adapter+API

And I think we should normalize paths by https://docs.obsidian.md/Plugins/Releasing/Plugin+guidelines#Use+%60normalizePath()%60+to+clean+up+user-defined+paths. Should be another issue

SettingDust commented 1 year ago

Should be fine in Obsidian 1.2.7

juangamnik commented 1 year ago

Should be fine in Obsidian 1.2.7

@SettingDust: So is this a feature finished or is there still something open (and if yes, what needs to be done?).

SettingDust commented 1 year ago

@SettingDust: So is this a feature finished or is there still something open (and if yes, what needs to be done?).

Need review from @coddingtonbear

juangamnik commented 1 year ago

Need review from @coddingtonbear

One question: why do you use fs.mkdir instead of Vault.createFolder(path:string) of the obsidian API?

SettingDust commented 1 year ago

One question: why do you use fs.mkdir instead of Vault.createFolder(path:string) of the obsidian API?

Sorry. I can't understand what are you saying. You can read reviews and comments for following current context

juangamnik commented 1 year ago

Sorry. I can't understand what are you saying. You can read reviews and comments for following current context

I resembled to the following statement of yours (above):

Sry. I forgot this PR. I've fixed this issue by using fs.mkdir. Have pushed

But I see in your commit from last week that you already changed the implementation in utils.ts to use the obsidian API instead of direct file system access.

Looks good to me (but I didn't test it).

How can I test this fork in my obsidian installation?

SettingDust commented 1 year ago

How can I test this fork in my obsidian installation?

Clone the repo and build

coddingtonbear commented 1 year ago

@juangamnik thanks for trying to help review this! It's a little easier for both you and the person who you're reviewing the code for if you go to the "Files Changed" tab and add comments on the relevant lines. Otherwise: it's a little difficult to piece together. Thanks for having a look, though!

sbtourist commented 1 year ago

Hello! What's the status of this PR? Is it ready to be merged and when will it be released? I'm also happy to help :)

SettingDust commented 1 year ago

Hello! What's the status of this PR? Is it ready to be merged and when will it be released? I'm also happy to help :)

Ready for merge

sbtourist commented 1 year ago

@SettingDust Nice, thanks, hopefully @coddingtonbear can merge and release it soon.

coddingtonbear commented 1 year ago

@sbtourist -- I think the only major thing preventing this from being merged/released is finding the time to install this locally and test it. If you'd be willing to take that on, that would accellerate things. Otherwise, it might be a couple weeks before I can find the time.

sbtourist commented 1 year ago

@coddingtonbear Sure I can do that, should be quick.

sbtourist commented 1 year ago

@SettingDust I tried building locally and I get:

> obsidian-local-rest-api@1.5.1 dev
> node esbuild.config.mjs

 > node_modules/proxy-addr/index.js:24:21: error: Could not resolve "ipaddr.js" (mark it as external to exclude it from the bundle, or surround it with try/catch to handle the failure at run-time)
    24 │ var ipaddr = require('ipaddr.js')
       ╵                      ~~~~~~~~~~~

 > node_modules/express/lib/application.js:16:27: error: Could not resolve "finalhandler" (mark it as external to exclude it from the bundle, or surround it with try/catch to handle the failure at run-time)
    16 │ var finalhandler = require('finalhandler');
       ╵                            ~~~~~~~~~~~~~~

2 errors
SettingDust commented 1 year ago

@sbtourist shouldn't relate to my changes. Difficult to say what's wrong with it. But it's fine in my local dev env

coddingtonbear commented 1 year ago

@sbtourist -- I believe what you might need to do is run npm i before running npm run dev; without doing that, you'll be missing dependencies.

coddingtonbear commented 1 year ago

These changes were merged (with some minor simplifications) as part of 94f97be. Thanks for your help with this, @SettingDust !

sbtourist commented 1 year ago

No, I had run it. Unfortunately I've been taken away by unexpected stuff and wasn't able to investigate more, but great to hear you managed to merge it! Thanks!

On Sun, 11 Jun 2023, 06:20 Adam Coddington, @.***> wrote:

@sbtourist https://github.com/sbtourist -- I believe what you might need to do is run npm i before running npm run dev; without doing that, you'll be missing dependencies.

— Reply to this email directly, view it on GitHub https://github.com/coddingtonbear/obsidian-local-rest-api/pull/44#issuecomment-1586022482, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZJ5YCLGQV5JWNXZ55XHTXKVISVANCNFSM6AAAAAASOB23RE . You are receiving this because you were mentioned.Message ID: @.***>