search

haskell-hvr / cassava

A CSV parsing and encoding library optimized for ease of use and high performance
http://hackage.haskell.org/package/cassava
BSD 3-Clause "New" or "Revised" License
223 stars 107 forks source link

headerOrder should take a Proxy #138

Closed ivan-m closed 7 years ago

ivan-m commented 7 years ago

Currently, the DefaultOrdered class' method headerOrder takes an argument whose value isn't used, just the type. Shouldn't a Proxy be used instead?

(This will, however, be a breaking change.)

hvr commented 7 years ago

I agree this would be more principled, but I'm hesitant at this point. I'll think about this...

ivan-m commented 7 years ago

Sure; this is a "if you're going to be making breaking changes anyway ..." issue.

ivan-m commented 7 years ago

How about if nothing else changing the generic definitions, since they're not exported?

hvr commented 7 years ago

@ivan-m can you make a PR to show what you mean? cleaning up internals if they don't leak into the API sounds good to me; assuming they have no measurable performance regression

ivan-m commented 7 years ago

Nevermind, selName takes a value and hence requires undefined, so we'd be converting to Proxy just to convert back to undefined.

ivan-m commented 7 years ago

I'm going to close this until/unless selName in GHC.Generics uses Proxy rather than undefined.

mbwgh commented 6 years ago

I actually just triggered Prelude.undefined because of this, by accidentally writing an instance like

instance DefaultOrdered Student where
  headerOrder Student{..} = header [...]

In hindsight, I should have noticed the unused variable, but I was just writing a hackish script. I believe the haddocks could be more clear about this. Currently, they only refer to how to call this function, but not to the caveats that may arise when implementing it.