silkapp / rest

Packages for defining APIs, running them, generating client code and documentation.
http://silkapp.github.io/rest
390 stars 52 forks source link

Nitpicking, mostly. #94

Closed mf59816 closed 9 years ago

mf59816 commented 9 years ago

This is a trivial comments-only patch. If you want to correct / ignore me on 056a071 you can have the other two commits individually, of course.

thanks-

hesselink commented 9 years ago

Thanks! The 'pendantic' and 'typos' commits looks good. I'm not sure about the one mentioning the void package, since I think there's no real difference between our definition and the one in the 'void' package: both contain no inhabitants other than bottom. What exactly are you trying to explain in that comment?

mf59816 commented 9 years ago

You are right, I was trying to explain this to myself, but after your clarification realize I didn't quite succeed...

Then my question would be "why do you introduce a new type if there is one already?" (-:

I can think of two reasons: (1) one extra line of code is better than one package dependency, and (2) your 'Void' has a constructor and the 'magic' de-constructor.

What about this:

-- (The void package also provides a similar type with the same name,
-- but without constructor and selector.  This makes ours slightly
-- more convenient to use.)

I'm interested if this is ok for you and possibly submit it again later, but I will revert the commit for now so you can move this out of the way.

Thanks for the feedback! -matthias

hesselink commented 9 years ago

The reason for defining it ourselves what indeed that it was only one data type, and avoided pulling in 'void' and its dependencies. I considered the downside, i.e. not being interoperable, but I doubt anyone wants to use a Void from one package in another. Also (but very minor) I like this definition slightly better than the recursive newtype or an empty data type.

The convenience ours is only useful internally, so I'm still not sure how useful it is to document this. Were you confused by it? If you were, perhaps we should just switch over to 'void' instead, which seems to be the de-facto standard.

Anyway, I've merged the first two commits and released rest-core-0.33.1.2 to hackage.

mf59816 commented 9 years ago

i don't think you should use void; the only thing that confused me was that i was afraid i had missed something.

i tried again, but please don't spend more time on this issue. if you like the new comment, you can keep it,otherwise please close this pull request anyway. i'm not confused any more. (-:

thanks!

bergmark commented 9 years ago

This is partially merged, is there anything left we want or can we close this?

mf59816 commented 9 years ago

yes, please close it. thanks-

bergmark commented 9 years ago

Thanks!