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.

Sign up for GitHub

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

Jump to bottom

Define our own "def" rather than the one from data-default-class #194

Closed
judah opened this issue May 10, 2018 · 2 comments
Closed

Define our own "def" rather than the one from data-default-class #194

judah opened this issue May 10, 2018 · 2 comments

Comments

@judah
Copy link
Collaborator

judah commented May 10, 2018

The data-default-class package is...somewhat controversial. The main criticism is that def doesn't satisfy any laws, so tends to be abused. See for example the following haskell-cafe thread: https://mail.haskell.org/pipermail/haskell-cafe/2018-May/129053.html

Indeed, from our experience using proto-lens on a larger team, we've found that data-default creeps into places it's not really intended for; e.g., using it for an integer value instead of an explicit 0.

However, for protobufs we do have a useful law, namely, encodeMessage def == "".

Note that the proto-lens library already reexports def directly: (Data.ProtoLens.Message and Data.ProtoLens)

I propose the following changes:

  • Remove all our dependencies on data-default-class (and data-default)

  • Change the Message class to define its own method def:

    class Message m where
    ...
    def :: m

Note that this is a breaking change, so it would cause a major version bump (i.e., proto-lens-0.4). Given that we just released 0.3 which has a fair bit of API churn, I'd probably rather wait a bit before doing another round. However, it should be fairly easy to write user code that's compatible with both versions, by writing import Data.ProtoLens (def).
@blackgnezdo
Copy link
Collaborator

If we are rolling our own, does it make sense to name it something else too? Maybe emptyMessage (contract to taste)? Transition can be arranged for with a wrapper module setting def = emptyMessage.

It may also be better to do two breaking releases back-to-back in hope that some people will have skipped the .3 release?

blackgnezdo pushed a commit that referenced this issue Aug 17, 2018
@fkm3
Update changelog for v0.2.0.0 (#194)
e40d2c4
judah added a commit to judah/proto-lens that referenced this issue Aug 23, 2018
@judah
Don't use data-default-class to build protos.
db2479a 
Fixes google#194.  `data-default` is problematic since it's (1) lawless and (2)
creeps into places it's not really intended for; e.g., using it for an integer
value instead of an explicit 0.  But protobufs *do* have a useful law, namely,
`encodeMessage def == ""`.

This change makes `Data.ProtoLens.Message` (and thus `Data.ProtoLens`, which reexports it):

- Add a `defaultMessage` method to `Message`.
- Redefine `def = defaultMessage` for convenience.

I'm not convinced this is the best long-term UX; suggestions welcome.
judah added a commit to judah/proto-lens that referenced this issue Aug 23, 2018
@judah
Don't use data-default-class to build protos.
80a2bc7 
Fixes google#194.  `data-default` is problematic since it's (1) lawless and (2)
creeps into places it's not really intended for; e.g., using it for an integer
value instead of an explicit 0.  But protobufs *do* have a useful law, namely,
`encodeMessage def == ""`.

This change makes `Data.ProtoLens.Message` (and thus `Data.ProtoLens`, which reexports it):

- Add a `defaultMessage` method to `Message`.
- Redefine `def = defaultMessage` for convenience.

I'm not convinced this is the best long-term UX; suggestions welcome.
judah added a commit to judah/proto-lens that referenced this issue Aug 23, 2018
@judah
Don't use data-default-class to build protos.
103f41b 
Fixes google#194.  `data-default` is problematic since it's (1) lawless and (2)
creeps into places it's not really intended for; e.g., using it for an integer
value instead of an explicit 0.  But protobufs *do* have a useful law, namely,
`encodeMessage def == ""`.

Before this change, the `Message` class had:

```
class Data.Default.Class.Default a => Message a where
    ...
```

After this change:

```
class Message a where
    defMessage :: a
    ...
```

We also provide a module `Data.ProtoLens.Default` which exports `def = defMessage`,
to make it easier to convert old code.

Also bumps versions; in particular, `proto-lens` is now at `v0.4`.
judah added a commit to judah/proto-lens that referenced this issue Aug 23, 2018
@judah
Don't use data-default-class to build protos.
c998f7e 
Fixes google#194.  `data-default` is problematic since it's (1) lawless and (2)
creeps into places it's not really intended for; e.g., using it for an integer
value instead of an explicit 0.  But protobufs *do* have a useful law, namely,
`encodeMessage def == ""`.

Before this change, the `Message` class had:

```
class Data.Default.Class.Default a => Message a where
    ...
```

After this change:

```
class Message a where
    defMessage :: a
    ...
```

We also provide a module `Data.ProtoLens.Default` which exports `def = defMessage`,
to make it easier to convert old code.

Also bumps versions; in particular, `proto-lens` is now at `v0.4`.
@judah judah mentioned this issue Aug 23, 2018
Merged
@judah judah closed this as completed in #210 Sep 3, 2018
judah added a commit that referenced this issue Sep 3, 2018
@judah
Don't use data-default-class to build protos. (#210)
8b23947 
Fixes #194.  `data-default` is problematic since it's (1) lawless and (2)
creeps into places it's not really intended for; e.g., using it for an integer
value instead of an explicit 0.  But protobufs *do* have a useful law, namely,
`encodeMessage def == ""`.

Before this change, the `Message` class had:

```
class Data.Default.Class.Default a => Message a where
    ...
```

After this change:

```
class Message a where
    defMessage :: a
    ...
```

We also provide a module `Data.ProtoLens.Default` which exports `def = defMessage`,
to make it easier to convert old code.

Also bumps versions; in particular, `proto-lens` is now at `v0.4`.
@mitchellwrosen
Copy link

mitchellwrosen commented Sep 30, 2018 •
edited
Loading

Just chiming in here, def is a useful concept for default options, etc. Problem is the data-default-class version ships with all kinds of crazy instances. Perhaps we should try to get a major version bump into it that removes basically all its instances, leaving only derived ones like (Default a, Default b) => Default (a, b). If that happens, then we can go back to all sharing the one class, instead of reinventing it everywhere :)

@mitchellwrosen mitchellwrosen mentioned this issue Sep 30, 2018
Open
@stephentetley stephentetley mentioned this issue Dec 12, 2020
Closed
19 tasks
avdv pushed a commit to avdv/proto-lens that referenced this issue Aug 9, 2023
@judah
Don't use data-default-class to build protos. (google#210)
f3db4e3 
Fixes google#194.  `data-default` is problematic since it's (1) lawless and (2)
creeps into places it's not really intended for; e.g., using it for an integer
value instead of an explicit 0.  But protobufs *do* have a useful law, namely,
`encodeMessage def == ""`.

Before this change, the `Message` class had:

```
class Data.Default.Class.Default a => Message a where
    ...
```

After this change:

```
class Message a where
    defMessage :: a
    ...
```

We also provide a module `Data.ProtoLens.Default` which exports `def = defMessage`,
to make it easier to convert old code.

Also bumps versions; in particular, `proto-lens` is now at `v0.4`.
ylecornec pushed a commit to ylecornec/proto-lens that referenced this issue Feb 19, 2024
@judah
Don't use data-default-class to build protos. (google#210)
daf40be 
Fixes google#194.  `data-default` is problematic since it's (1) lawless and (2)
creeps into places it's not really intended for; e.g., using it for an integer
value instead of an explicit 0.  But protobufs *do* have a useful law, namely,
`encodeMessage def == ""`.

Before this change, the `Message` class had:

```
class Data.Default.Class.Default a => Message a where
    ...
```

After this change:

```
class Message a where
    defMessage :: a
    ...
```

We also provide a module `Data.ProtoLens.Default` which exports `def = defMessage`,
to make it easier to convert old code.

Also bumps versions; in particular, `proto-lens` is now at `v0.4`.
ylecornec pushed a commit to ylecornec/proto-lens that referenced this issue Feb 19, 2024
@judah
Don't use data-default-class to build protos. (google#210)
55ace15 
Fixes google#194.  `data-default` is problematic since it's (1) lawless and (2)
creeps into places it's not really intended for; e.g., using it for an integer
value instead of an explicit 0.  But protobufs *do* have a useful law, namely,
`encodeMessage def == ""`.

Before this change, the `Message` class had:

```
class Data.Default.Class.Default a => Message a where
    ...
```

After this change:

```
class Message a where
    defMessage :: a
    ...
```

We also provide a module `Data.ProtoLens.Default` which exports `def = defMessage`,
to make it easier to convert old code.

Also bumps versions; in particular, `proto-lens` is now at `v0.4`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@judah @mitchellwrosen @blackgnezdo