search

evancz / url-parser

Parse URLs into nicely structured data
http://package.elm-lang.org/packages/evancz/url-parser/latest/
BSD 3-Clause "New" or "Revised" License
114 stars 29 forks source link

Tests #10

Closed sporto closed 8 years ago

sporto commented 8 years ago

This library is quite important for people building SPAs with Elm. As a string parsing library I think this lib needs tests.

I will be happy to put an initial PR if you would be happy to review it and merge it. Let me know, thanks.

process-bot commented 8 years ago

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

ericgj commented 8 years ago

I agree - including tests of the issue that PR #5 fixes. Haven't used elm-test yet myself but would be happy to learn/help.

evancz commented 8 years ago

I think that'd be nice to have. I'd start out with something simple that's easy to review so things don't go off in a weird direction.

I don't think keeping this open makes it any more done, so if you want to write some tests let's follow up in a PR or talk more here if some coordination is needed before that.

Note: You say roughly, I'll do the work if you'll merge it (i.e. "if you would be happy to review it and merge it") and I just want to point out that no responsible maintainer would give a promise of merging stuff in sight-unseen from someone they have never worked with. This issue is also so vague that there's no real feedback I can give on your plan. Unit tests? Fuzz tests?

sporto commented 8 years ago

@evancz I said that I will do work if you review it ...` not because I'm expecting a promise of merging, I don't expect this at all. I said this because I have seen several people opening PR in Elm related stuff and they just go unreviewed for weeks. I know you are busy, but I just wanted to get some affirmation that you would look at the PR and merge if good.

sporto commented 8 years ago

@evancz My plan for test would be:

I'll start with a simple PR as you say

evancz commented 8 years ago

Cool, sounds good to me!

About the delay on getting feedback, I tried to explain why it's that way in this talk in the part about batching work. There is nothing wrong with code sitting for a while. Making a language is a 30+ year project, so I prefer to do things well than to do things right-this-second. It's explained better in the talk though!

sporto commented 8 years ago

Master is now tracking version 2 for Elm 0.18. It probably doesn't make sense making test for version 1 at this stage, does it? In this case I will just wait for elm-test to have a compatible version with Elm 0.18.

evancz commented 8 years ago

Yeah, there are definitely bad things in the old version (like caring if it starts with a slash). It only makes sense to test the new one.