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 missing --include flag & linksToInclude option #105

Closed
wants to merge 1 commit into from

Conversation

marapper
Copy link
Contributor

You have --include flag description in README but part with implementation is kinda missing. As I suppose it should work like same flag in other tooling. Or maybe even like .ignore files - with ! at start of strings.

We need something like that in our small project in which I implement your lib. So I hope it's ok, my accidentaly PRs.

Here the issue #44

@JustinBeckwith
Copy link
Owner

Thanks for the PR! So to be honest, I'm kind of torn on this one. I'm trying to be a little careful with the number of flags we add. Can you share under what circumstances you'd use something like this?

We've found that having skip and then scanning the whole site generally suits our needs. When would you use something like this?

@marapper
Copy link
Contributor Author

marapper commented Nov 17, 2019

Yes, I understand your concerns.

But I think exclude flags is not powerful enough without force includes. Thats why all .ignore files and test-properties in tooling have both.

Two simple wxample for crawling:

  1. If we want to check all internal links but not externals. So we could skip all links (/.*/) but allow internal links by domain.

  2. We can forbid to check all youtube-domain but allow /youtube.com\/watch.*/

Also you already have --include, -i description in README.md

@marapper
Copy link
Contributor Author

Are you still have doubts?

@JustinBeckwith
Copy link
Owner

So I think I'm ready to call it - I don't think this is needed. Recently we landed #120, which lets you decide if a link is skipped by passing it through a function. So for sufficiently advanced scenarios, you can drop down to the filter function to really control what comes back.

I'm going to go ahead and close this out for now - but really, thank you for the POC.

@marapper
Copy link
Contributor Author

Yes, it's totally reasonable. include description in README.md is no longer needed may be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants