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

Refactor and test #26

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

spejman
Copy link

@spejman spejman commented Apr 28, 2022

A proposal to refactor the code in different files split per functionality:

  • src/logger.js -> everything related to logging.
  • src/cli.js -> parse input, printing help, managing args.
  • src/pages.js -> modifying and converting pages content.
  • src/filesystem.js -> dealing with the files and directories.

Also added two files with a small set of tests that ensures the script behaves as expected, and also speeds up the development because you can easily add a test with the bug that is failing and run the tests after each code change to ensure it's fixed.

@connertennery
Copy link
Owner

@spejman Wow! This is incredible. Thank you so much for all of the work you've put into this! The reason I hadn't split it up originally (and why I stopped working on the TypeScript branch) is because I wanted to make using the script very accessible with a single curl. It's definitely gained more usage than I ever expected so I guess it would be best to create an NPM package. The testing suite has become a necessity so I can't thank you enough for that as well.

I'll review this in-depth tonight and likely complete the pull request then.

@connertennery
Copy link
Owner

@spejman I'm having issues getting this to run - startConversion in main.js is not defined, fs in cli.js is not defined, regex[1] is failing in pages.js.

I think this is a good idea, but it's going to need some work before it can be merged. I'll take a closer look as soon as I can but I'll be really busy for a while so I can't make any guarantees to my timeline.

Thank you!

@spejman
Copy link
Author

spejman commented May 28, 2022

Hi @connertennery ! Sorry I didn't have the notifications properly configured and didn't get the notification of your comments.

I just checked your comments and noticed that my last merge 'main' broke the code, I just fixed it, it should work as expected now.

@spejman
Copy link
Author

spejman commented May 28, 2022

I also just used decodeURI like a1a76db (Note that I didn't use decodeURI on image URLs because otherwise images are not automatically shown in Obsidian).

Thanks!

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