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

feat: add new listr function with add capability #144

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chamabreu
Copy link
Contributor

BREAKING CHANGE: It is now possible to give a folder as file argument e.g.: md2pdf $(pwd)

This works recursive. The watcher will listen for all add and change events in all subfolders, but only for markdown files.

For compatibility, a legacy function is kept alive, to handle multiple input paths, like ./**/*.md, so the old cli app works like before.

BREAKING CHANGE: It is now possible to give a folder as file argument
e.g.: md2pdf $(pwd)

This works recursive. The watcher will listen for all add and change
events in all subfolders, but only for markdown files.

For compatibility, a legacy function is kept alive, to handle multiple
input paths, like ./**/*.md, so the old cli app works like before.
@chamabreu
Copy link
Contributor Author

Hi,
related to #137 , this is my example how this could be implemented.

Refactored on the way a few things.

Would be nice, if we can pull this in.
If not, no Problem, I will work then only on my fork.

Copy link
Owner

@simonhaenisch simonhaenisch left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this! I've had a quick look, and it's a lot of changes overall... finding it a bit hard to follow through. Would have been nice if you could have split it into separate commits for the individual changes so it would be easier to understand and reason about each change 🤓

Anyway, one thing I would like to propose is to allow setting a folder with a -r/--recursive flag instead, so maybe this could get away as a non-breaking change (:

But not sure I would go ahead with this PR as-is, e.g. I find the "convert factory" part a bit too abstract.

@chamabreu
Copy link
Contributor Author

Thanks for the feedback.

I implemented the convertFactory for that it is not more necessary to push all the configs and args around.
But maybe this could be overpowered now, I have to review it myself.

If you are willing to merge it, if I develop it further to your coding style, I will do.
If you dont want this feature in your repo, I will stick to my fork.

Just tell me.
I would do:
Refactor the factoryFunction (remove)

Dunno if its necessary to add that -r flag.
At the state now, it is not really breaking, is it?
Because i kept the posibility to pass a glob pattern.

But I had the idea, that a user could give a folder, and set the recursive depth.
Maybe this and the -r flag could go in a next PR/Feature.

@chamabreu
Copy link
Contributor Author

Pushed a commit where convertFactory is removed.

I would need to dig deeper in CLI flags, to get to know how to implement the -r flag.
Is this mandatory for you?
I think, because the program works like before, with the ./*.md and ./**/*.md inputs, the folder functionality as is now, is just added, and does not replace or interrupt the old approach.

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.

None yet

2 participants