search

gomarkdown / markdown

markdown parser and HTML renderer for Go
Other
1.41k stars 173 forks source link

Add IsSafeURLOverride #252

Closed onionltd closed 2 years ago

onionltd commented 2 years ago

If set, the hook overrides the default IsSafeURL and allows users of the library to decide which URIs are safe to keep. This asked for some refactoring which I made into separate commits. I think the PR is quite big and thought it would be best to review it as two separate PRs. However, I didn't communicate my intentions and this was met with misunderstanding. This time around I'm submitting the branch as is. I have left some TODOs which I will take care once this gets some eyeballs.

miekg commented 2 years ago

This seems to do way more and adds a badly named "utils" pkg ...

On Fri, 29 Jul 2022, 20:24 Onion Ltd, @.***> wrote:

If set, the hook overrides the default IsSafeURL and allows users of the library to decide which URIs are safe to keep. This asked for some refactoring which I made into separate commits. I think the PR is quite big and thought it would be best to review it as two separate PRs. However, I didn't communicate my intentions and this was met with misunderstanding https://github.com/gomarkdown/markdown/pull/251. This time around I'm submitting the branch as is. I have left some TODOs which I will take care once this gets some eyeballs.

You can view, comment on, or merge this pull request online at:

https://github.com/gomarkdown/markdown/pull/252 Commit Summary

File Changes

(11 files https://github.com/gomarkdown/markdown/pull/252/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/gomarkdown/markdown/pull/252, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIW2KJ2QHUZMQF2QZM2LVWQOV3ANCNFSM55BSKX5Q . You are receiving this because you are subscribed to this thread.Message ID: @.***>

onionltd commented 2 years ago

This seems to do way more and adds a badly named "utils" pkg ...

I saw your comment. I don't mind splitting the code in two separate packages textutil and astutil, if that's what you think is the right way.

kjk commented 2 years ago

It's not so much about the name but the whole idea of refactoring into internal package and bundling that change with unrelated change for IsSafeURL

Yes, we have a little bit of code duplication and that's unfortunate. But also "A little copying is better than a little dependency." as per https://go-proverbs.github.io/

On the whole, I don't think the amount and nature of duplicated code warrants this refactoring. Especially given that I consider this library more or less done i.e. neither me nor anyone else is actively working on making significant changes.

Bottom line: making IsSafeURL configurable is an acceptable addition but please undo the refactoring into internal package.

kjk commented 2 years ago

BTW: I'm a minimalist, pragmatist and not dogmatist.

Minimalist: less code is better than more code. Less packages is better than more packages.

Pragmatist and not dogmatist: things don't have to be perfect.

In retrospect, I probably made a mistake in splitting this code into multiple packages. Because less packages is better than more packages. It's too late to change that.

As it pertains to the little code duplication we have: I don't want to add more packages because I'm a minimalist.

We have a "leaf" package ast where we can put functions that are used in more than one package. Does isAlnum belong, logically, in ast package? It doesn't. But I'm a pragmatist and I'm ok with putting it there..

Either way such refactoring doesn't belong in IsSafeURL change.

onionltd commented 2 years ago

Thank you for your feedback. I have dropped the commits that do not pertain to the subject. There isn't as much code duplication after all.

kjk commented 2 years ago

Thanks!