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

feat: support escape slash in wikilinks #290

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pprotas
Copy link

@pprotas pprotas commented Jan 13, 2024

Ref: #288

The pipe operator used in WikiLinks can now be escaped with a backwards slash \.

Ref: artempyanykh#288

The pipe operator used in WikiLinks can now be escaped with a backwards slash `\`.
@artempyanykh
Copy link
Owner

artempyanykh commented Jan 15, 2024

Thanks for submitting this PR! I'll have a look shortly.

@pprotas
Copy link
Author

pprotas commented Jan 21, 2024

Hi sorry for the fails, I should have ran the tests locally but I am not familiar with this particular stack. I managed to set up dotnet on my macbook and I'm now taking a look at the tests.

The problem with the runs above is that the slash | was being encoded before \\|, so swapping the order of those two fixed that issue.

There is another problem, on Tests/MiscTests.fs:100 there is an expect that decoded actual and compares it to original. With the current implementation, this will not work because the escape slashes are lost during the encoding process, and during decoding on line 100 there is no way to know whether the escaped or non-escaped version of | should be used.

So my thinking is, we can maybe remove that expect on line 100 because line 99 is the actual test, and it looks to me like line 100 is just an extra precaution. While I think such an extra precaution is nice, it makes this change a bit harder. On the other hand, I am unfamiliar with the codebase so my thinking could be a bit shortsighted here so I would value your opinion on the matter. Thanks.

@artempyanykh
Copy link
Owner

Hi @pprotas! If I understood correctly #288, you wanted | to be escaped \| rather than url-encoded. With you current change, auto-completion would still produce a string with url-encoded pipe, which defeats the purpose.

EncodeForWiki probably needs to look like this:

     member this.EncodeForWiki() : string =
-        let replacement = [| "#", "%23"; "[", "%5B"; "]", "%5D"; "|", "%7C" |]
+        let replacement = [| "#", "%23"; "[", "%5B"; "]", "%5D"; "|", "\\|" |]

And, you'd also need to implement DecodeFromWiki to do the reverse, rather than piggy-backing on UrlDecode.

In terms of tests, I expect encoding to be lossless, iow decode(encode(original) should be equal to original. It can stay this way if you modify DecodeFromWiki appropriately.

@artempyanykh artempyanykh added the needs-further-input Further information is required from the reporter label Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-further-input Further information is required from the reporter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants