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 test option for additional checks #10

Merged
merged 5 commits into from
May 4, 2023

Conversation

hanneskuettner
Copy link
Contributor

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Add a test option that is converted using hast-util-is-element and adds additional matching capabilities to exclude some external link elements as discussed in #3.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels May 4, 2023
@wooorm
Copy link
Member

wooorm commented May 4, 2023

Can you move the is = convert() bit up, so that it is called once: e.g., directly inside rehypeExternalLinks. Now it’s doing unneeded work.

I think is can be called in the node.tagName === 'a' branch? That way, if there is a test that says the element shouldn’t be converted, we don’t need to do extra work of checking protocols.

Lastly, would it be an idea to default to 'a' as a test?
That way, people could allow ['a', 'area']. Or include custom elements (weird but ok)?
Not sure if useful. What do you think?

@hanneskuettner
Copy link
Contributor Author

Can you move the is = convert() bit up, so that it is called once: e.g., directly inside rehypeExternalLinks. Now it’s doing unneeded work.

Good point!

I think is can be called in the node.tagName === 'a' branch? That way, if there is a test that says the element shouldn’t be converted, we don’t need to do extra work of checking protocols.

Another good point, I'll also do that!

Lastly, would it be an idea to default to 'a' as a test?
That way, people could allow ['a', 'area']. Or include custom elements (weird but ok)?
Not sure if useful. What do you think?

I thought about that as well. This could maybe lead to some unexpected behavior, since the test would then have to reimplement checking for an a tag in the simple case of just wanting to exclude some domains for example.
But maybe that is okay with an additional note in the docs?
It would certainly make this plugin more powerful.

@wooorm
Copy link
Member

wooorm commented May 4, 2023

The only real benefit would be that area elements are supported.

Potentially custom elements too, but then it gets weird that rel and target are applied with the same semantics as a, because custom elements probably work differently.

Perhaps area could also just be added extra here for everyone: node.tagName === 'a' || node.tagName === 'area'. Later.

I think I’m fine with the current state!

test.js Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2fab32f) 100.00% compared to head (791bc63) 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #10   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          135       144    +9     
=========================================
+ Hits           135       144    +9     
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@wooorm wooorm changed the title Add test option that allows additional matching of link elements Add test option for additional checks May 4, 2023
@wooorm wooorm merged commit 4a7a050 into rehypejs:main May 4, 2023
@github-actions
Copy link

github-actions bot commented May 4, 2023

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

@wooorm
Copy link
Member

wooorm commented May 4, 2023

Released in 2.1.0! Thank you!

@hanneskuettner
Copy link
Contributor Author

Thanks for your fast handling. That was a joy!

@hanneskuettner hanneskuettner deleted the feat/test-function branch May 4, 2023 13:20
fuxingloh referenced this pull request in fuxingloh/contented May 14, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[rehype-external-links](https://github.com/rehypejs/rehype-external-links)
| [`^2.0.1` ->
`^2.1.0`](https://renovatebot.com/diffs/npm/rehype-external-links/2.0.1/2.1.0)
|
[![age](https://badges.renovateapi.com/packages/npm/rehype-external-links/2.1.0/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/rehype-external-links/2.1.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/rehype-external-links/2.1.0/compatibility-slim/2.0.1)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/rehype-external-links/2.1.0/confidence-slim/2.0.1)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>rehypejs/rehype-external-links</summary>

###
[`v2.1.0`](https://github.com/rehypejs/rehype-external-links/releases/tag/2.1.0)

[Compare
Source](https://github.com/rehypejs/rehype-external-links/compare/2.0.1...2.1.0)

##### Add

-
[`4a7a050`](https://github.com/rehypejs/rehype-external-links/commit/4a7a050)
Add `test` option for additional checks
by [@&#8203;hanneskuettner](https://github.com/hanneskuettner) in
[https://github.com/rehypejs/rehype-external-links/pull/10](https://github.com/rehypejs/rehype-external-links/pull/10)

**Full Changelog**:
rehypejs/rehype-external-links@2.0.1...2.1.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/BirthdayResearch/contented).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS43MS4wIiwidXBkYXRlZEluVmVyIjoiMzUuNzEuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤞 phase/open Post is being triaged manually
Development

Successfully merging this pull request may close these issues.

3 participants