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 astro plugin #298

Merged
merged 9 commits into from
Oct 17, 2023
Merged

Add astro plugin #298

merged 9 commits into from
Oct 17, 2023

Conversation

DaniFoldi
Copy link
Contributor

Hi 👋,

Following up my issue #201 I implemented a plugin that I believe matches the behaviour of Astro quite closely, following the docs for exact patterns.

The only remaining issue is ignoring the astro:content module, as it is resolved at build/runtime by astro, and not a separate dependency. Happy to fix if you could just point me in the right direction.

Copy link
Collaborator

@webpro webpro left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

I happen to have built an Astro site recently, so this was the perfect case to try it out.

Just a bit of clean up would be great. I think more irrelevant HTML and CSS can be removed. If you could cut some of that away, that would be great. Either way, I'll be happy to merge this.

The issue with astro:content is a little bit tricky, but I'll have a fix ready after the merge. If you like, you can also include it in this PR and add a colon (:) to the second group of the regex on this line: https://github.com/webpro/knip/blob/6cd5a1d4e9575ff2dee60e1ca6cc47cbf7788acd/src/util/modules.ts#L63

fixtures/plugins/astro/.vscode/extensions.json Outdated Show resolved Hide resolved
fixtures/plugins/astro/.vscode/launch.json Outdated Show resolved Hide resolved
fixtures/plugins/astro/README.md Outdated Show resolved Hide resolved
fixtures/plugins/astro/knip.ts Outdated Show resolved Hide resolved
fixtures/plugins/astro/knip.ts Show resolved Hide resolved
fixtures/plugins/astro/package-lock.json Outdated Show resolved Hide resolved
fixtures/plugins/astro/public/favicon.svg Outdated Show resolved Hide resolved
fixtures/plugins/astro/src/content/blog/third-post.md Outdated Show resolved Hide resolved
src/plugins/astro/index.ts Outdated Show resolved Hide resolved
@DaniFoldi
Copy link
Contributor Author

Thanks for your prompt response! I've implemented all suggestions from your review, and was about to also change the test (as astro:content is no longer reported as unlisted - but test are somehow worse now 😅, both locally and on a fresh clone in Codespaces, so I think it's best if I wait for your input on the current state before I mess up even more - my understanding of the codebase is still fairly limited.

@webpro
Copy link
Collaborator

webpro commented Oct 17, 2023

I've added two commits for the remaining bits :)

@webpro
Copy link
Collaborator

webpro commented Oct 17, 2023

Next step (for me) is to rebase from the main branch, because I did a major refactor just the other day (you couldn't know).

@webpro webpro merged commit 50dd048 into webpro-nl:main Oct 17, 2023
10 checks passed
@DaniFoldi
Copy link
Contributor Author

Many thanks for your kind help! ✂️ 🚀

@DaniFoldi DaniFoldi deleted the astro-plugin branch October 17, 2023 13:12
@webpro
Copy link
Collaborator

webpro commented Oct 17, 2023

🚀 This pull request is included in v2.34.0. See Release 2.34.0 for release notes.

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