search

jfmengels / elm-review-documentation

Provides elm-review rules to help with the quality and correctness of your Elm project's documentation
https://package.elm-lang.org/packages/jfmengels/elm-review-documentation/latest/
BSD 3-Clause "New" or "Revised" License
6 stars 2 forks source link

Docs.ReviewLinksAndSections could allow for “absolute path” links to other packages #17

Open agj opened 1 year ago

agj commented 1 year ago

Currently, the Docs.ReviewLinksAndSections rule complains if I have a link such as /packages/elm/core/latest/Basics#%7C%3E linking to the documentation for |>, and wants me to convert it to a full URL starting from the protocol.

My observation is that the “absolute path” pattern is useful because it not only works in the official Elm packages website, but also in dmy's fork, without taking you out of the website.

When not to enable this rule:

Maybe this doesn't make sense for people who generate their documentation and host it somewhere else. Not sure if there's people doing that, though. Otherwise, I think it should be pretty safe?

I am looking for:

Much love for elm-review and all the useful rules!

jfmengels commented 1 year ago

Hey @agj 👋

I'm slightly confused about a few things, I hope you don't mind me asking for more details/clarification :)

Your issue is that you're trying to have a link like /packages/elm/core/latest/Basics#%7C%3E but the rule is forcing you to link to that thing through https://package.elm-lang.org/packages/elm/core/latest/Basics#%7C%3E, is that correct? And your suggestion is to allow the /packages/elm/core/latest/Basics#%7C%3E pattern?

That's what I'm understanding from your issue, but maybe i'm misunderstanding. I'm slightly confused because I'm not entirely sure what you mean with "absolute path". That to me refers more to "https://..." but from your issue I understand the opposite. So I could use some clarification 😅

agj commented 1 year ago

Hi, yes, that's correct. What I meant by “absolute path” is one that begins with a / character. I'm not sure what the correct terminology is there, so I could've been more explicit on that aspect.

agj commented 1 year ago

I used that term because at least that's how you would call it on a Unix-like system. It's not relative because a relative URL is one that begins without the /, such as some/subdir/file.txt.

agj commented 1 year ago

Looks like it is indeed called an absolute path! 🤷

jfmengels commented 1 year ago

Aha, absolute path (/...) vs absolute URI (https://...). Okay, that makes sense.

So the question is: should the rule make it okay (or even preferably) to use /packages/... instead of https://package.elm-lang.org/packages/...?

I have to admit I'm not sure there are many downsides to that. It should work (someone please confirm?) on the official packages website, as well on dmy's version. And I'd be happy to have this rule be friendlier to dmy's version (which is really nice).

I do wonder about whether it would work on (the online and offline versions of)elm-doc-preview (https://elm-doc-preview.netlify.app/Review-Fix?repo=jfmengels%2Felm-review&version=master)

As for people hosting it on their website, that's not a (current) problem, as they probably wouldn't use this rule anyway? Not entirely sure.

I'd be okay with this change, and probably even as the default, if it turns out it works just as well (would love some opinions and testing on this).

agj commented 1 year ago

Hmm, yeah, it doesn't look like it would work on elm-doc-preview. That's one downside I hadn't thought of.

I'll try and write a PR to test this out (during the weekend, though, I think,) although it would be my first time touching elm-review rules code, so wish me luck!

jfmengels commented 1 year ago

I think it might. I think the website intercepts some link clicks to change the url, so it might work. Luckily it's testable without publishing a new version of a package 😊

Thanks for willing to make the PR. Best of luck to you 😄 just make sure you're adding / updating tests and it I'll probably go well 😊

I'm busy this weekend and next week, so it's possible I won't be super responsive. But feel free to take your time 👍

agj commented 1 year ago

I think it might. I think the website intercepts some link clicks to change the url, so it might work.

Nice, I wasn't aware of that.

Thanks for willing to make the PR. Best of luck to you 😄

Thanks! 🙏

I'm busy this weekend and next week, so it's possible I won't be super responsive. But feel free to take your time 👍

Sure, there's no rush either way.