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

support wildcard domain #33

Closed
wants to merge 3 commits into from
Closed

Conversation

tangkunyin
Copy link

@tangkunyin tangkunyin commented Aug 20, 2024

check list

  • Add test cases for the changes.
  • Passed the CI test.

Description

Support wildcard domain, that means subdomain also supported well, such as www or en.

Now we can just configure only exclude.com and it will be worked on *.exclude.com

Additional information

No!

Copy link
Member

@uiolee uiolee left a comment

Choose a reason for hiding this comment

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

It won't pass unit tests. And You need to add test cases

- **exclude** - Exclude hostname. Specify subdomain when applicable, including `www`.
- 'exclude1.com' does not apply to `www.exclude1.com` nor `en.exclude1.com`.
- **exclude** - Exclude hostname. Subdomain also supported well, such as `www` or `app`.
- 'exclude1.com' now apply to `www.exclude1.com` and `en.exclude1.com` in this [commit](https://github.com/tangkunyin/hexo-filter-nofollow/commit/cf05a07054536dfaa36f986493e678252f31ad45)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- 'exclude1.com' now apply to `www.exclude1.com` and `en.exclude1.com` in this [commit](https://github.com/tangkunyin/hexo-filter-nofollow/commit/cf05a07054536dfaa36f986493e678252f31ad45)
- 'exclude1.com' apply to `www.exclude1.com` and `en.exclude1.com`.

@@ -12,11 +12,11 @@ function isExternal(url, config) {

if (exclude && exclude.length) {
for (const i of exclude) {
if (host === i) return false;
if (i === host || (host && host.endsWith(i))) return false;
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary repetition

}
}

if (host !== sitehost) return true;
if (sitehost !== host && (host && !host.endsWith(sitehost))) return true;
Copy link
Member

Choose a reason for hiding this comment

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

For host to be null, it is recommended to use early return

@@ -1,6 +1,6 @@
{
"name": "hexo-filter-nofollow",
"version": "2.0.2",
"version": "2.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

No need to modify it

@uiolee
Copy link
Member

uiolee commented Sep 11, 2024

#12 Some users may want a more precise match, and using wildcards by default may not be a good idea.

@tangkunyin
Copy link
Author

#12 Some users may want a more precise match, and using wildcards by default may not be a good idea.

Ok, got it. Then I will close this PR.

@tangkunyin tangkunyin closed this Sep 12, 2024
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