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

Add candidateFilters option #107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

hankliu62
Copy link

No description provided.

@hankliu62
Copy link
Author

hankliu62 commented Jun 25, 2018

@luin @haroldtreen Please help review, and give some suggestions, thanks.

@haroldtreen
Copy link
Collaborator

This looks like an interesting option @hankliu62 👍 . What do you think about calling it candidateFilters? Filters just accept a single candidate, so it makes a bit more sense to me.

Curious if @luin has thoughts because I know less about the history of this module 😅 .

README.md Outdated
@@ -81,6 +81,23 @@ read(url, {
});
```

- `candidatesFilters` which allow set your own filters for candidate tags.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would read better as.

which allows you to set your own filters for candidate tags.

README.md Outdated
read(url, {
candidatesFilters: [
function (obj) {
if (obj.tagName === 'ARTICLE' && elem.getAttribute('type') === 'video') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe could include a comment explaining what this filter does?

eg.

// ...
    // Filter any article tags with a type of "video"
    function(obj) {
       // ...
    }

README.md Outdated
```javascript
read(url, {
candidatesFilters: [
function (obj) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

obj doesn't describe what this function is passed. Maybe something like candidateNode?

README.md Outdated
read(url, {
candidatesFilters: [
function (obj) {
if (obj.tagName === 'ARTICLE' && elem.getAttribute('type') === 'video') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also think elem is not defined in this example. I think you meant to make it the same as obj?

@hankliu62
Copy link
Author

@haroldtreen, thanks for all your help! I've updated PR according to your instructions. Let me know if I've missed something.

@hankliu62 hankliu62 changed the title Add candidatesFilters option Add candidateFilters option Jul 6, 2018
@haroldtreen
Copy link
Collaborator

Thanks for the updates @hankliu62 !

Only last thing I would consider is bumping the version in package.json to 3.1.0 to indicate a new feature has been added. Also note the new feature in CHANGELOG.md.

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