Fix escaping for URL fragments (#103)

ndmitchell / hoogle

Haskell API search engine

http://hoogle.haskell.org/

Other

753 stars 137 forks source link

Fix escaping for URL fragments (#103) #104

Closed hdgarrood closed 9 years ago

hdgarrood commented 9 years ago

Use the same escaping rules that Haddock does, to ensure that links to specific types or values in Haddock documentation (eg, mapM_, (:+)) work correctly.

ndmitchell commented 9 years ago

Thanks for taking the time to track it down properly! This looks a lot better. Two tweaks (happy for you to rebase this patch, add a follow up pull request, or add them to this patch as you prefer):

Can you please add a comment saying where isLegal was taken from originally, so it's easy to find in future if we need it.
Can you please add a test to the bottom of the Action.Search module. Just tweak the map test on line 62 to add mapM_ or similar.

Btw, if you made any (even very draft) notes while figuring out how to run the tests or debug stuff about Hoogle, feel free to add them in hoogle/docs/Developing.md. I have a vague intention to add something there at some point. But if it was obvious enough and you didn't need to, then don't worry.

hdgarrood commented 9 years ago

It was mostly obvious enough. I was a little surprised at first when I put in --database=output/base.hoo and got back something about not being able to find output/output/base.hoo.hoo but that was the only thing.

To run the tests, do I need to run hoogle generate and then hoogle test?

hdgarrood commented 9 years ago

I did that and it seemed to work.

ndmitchell commented 9 years ago

Thanks, all looks excellent. This will automatically deploy to hoogle.haskell.org at 8pm GMT tonight.

You are correct, generate and then test is the magic sequence.

The --database thing confused me massively too when I ran into it yesterday, I'd say that was a bug. I think I defaulted to output because it was handy at some point, and I append .hoo since I used to use multiple files for a database, so .hoo was only one of them. Both are no longer true, so --database should point at the database without any funny business (happy to accept a patch, or I'll fix it one day when it annoys me, certainly before I release Hoogle 5).