Skip to content
This repository has been archived by the owner on Feb 20, 2019. It is now read-only.

Improve custom pickler/unpickler generators #419

Merged
merged 3 commits into from
May 6, 2016

Conversation

jvican
Copy link
Member

@jvican jvican commented May 5, 2016

This PR improves how custom pickler/unpickler generators are created in both
scala pickling and third-party libraries that want to define their own for
concrete types.

It also improves error reporting significantly. Now we have concrete exceptions
for every kind of error that may happen. This improvement helps readability and
also gives a pretty straightforward idea of what could go wrong.

List of changes:

Add custom runtime generators for Either

* Add custom runtime generators for Either and tests checking the new
 custom runtime generation.

* Make `AnyPicklerUnpickler` deal with more corner cases.

* Modify PicklingProtocol in sandbox-test and reuse it for both formats.

* Add error for disabled runtime generation.

* Improve the generator helper, make some renamings and add more
 documentation.

Refactor runtime custom picklers

* Remove some todos and make a better distinction between the runtime
 custom picklers and the runtime pickler registry. Merge the latter one
 in the `DefaultRuntimePicklerGenerator`.

* Improve some docs w.r.t. the runtime custom picklers.

* Make the `RuntimePicklerRegistry` more general for future custom
 picklers (like Either).

* Remove annoying prints when running tests.

Improve error reporting

* Keep compatibility with the same old API but improve significantly the
 way we report errors factoring out the most important ones and
 changing some of the error messages.

* Factor out the functionality to register pickler generators (to be
 used at runtime) and make easier to add new ones for the missing types.

* In tests, changes make sure that scala pickling is imported and that the
 deprecated method `generate` for Pickler and Unpickler is not used.

* Apply minor changes throughout scala pickling.

* Keep compatibility with the same old API but improve significantly the
  way we report errors factoring out the most important ones and
  changing some of the error messages.

* Factor out the functionality to register pickler generators (to be
  used at runtime) and make easier to add new ones for the missing types.

* In tests, changes make sure that scala pickling is imported and that the
  deprecated method `generate` for Pickler and Unpickler is not used.

* Apply minor changes throughout scala pickling.
jvican added 2 commits May 5, 2016 17:27
 * Remove some todos and make a better distinction between the runtime
   custom picklers and the runtime pickler registry. Merge the latter
   one in the `DefaultRuntimePicklerGenerator`.

 * Improve some docs w.r.t. the runtime custom picklers.

 * Make the `RuntimePicklerRegistry` more general for future custom
   picklers (like Either).

 * Remove annoying prints when running tests.
* Add custom runtime generators for Either and tests checking the new
  custom runtime generation.

* Make `AnyPicklerUnpickler` deal with more corner cases.

* Modify PicklingProtocol in sandbox-test and reuse it for both formats.

* Add error for disabled runtime generation.

* Improve the generator helper, make some renamings and add more
  documentation.
@jvican jvican force-pushed the improve-error-handing-and-generation branch from fff4c7e to 3bc60b8 Compare May 5, 2016 15:30

private[pickling] object Feedback {

def failedGenerationMsg(x: String, y: String, z: String) =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use different arg names than x/y/z? I'll be confused everytime I look at this method :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x= tpe, z = lookedFor, y = goal

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll address this in a follow-up PR. Sorry! I was very haskell-ish (using a lazy-strategy).

@jsuereth
Copy link
Contributor

jsuereth commented May 6, 2016

This looks pretty great, nice work! A few comments (mostly useless), but a few questions as well. Nothing that has to be handled now.

@jsuereth jsuereth merged commit 7d4c351 into scala:0.11.x May 6, 2016
@jvican jvican deleted the improve-error-handing-and-generation branch May 6, 2016 15:28
@jvican
Copy link
Member Author

jvican commented May 6, 2016

Thanks @jsuereth 😄 I'll address some stuff in my next PR. Currently, I'm trying to solve the scalajs problem and try these new changes with my custom picklers for spores! But I'll be doing this soon!

@jvican jvican added this to the 0.11.0-M1 milestone May 9, 2016
jvican added a commit to jvican/pickling that referenced this pull request May 25, 2016
* Add a new unrecognized tag error.

* Abstract over `NoTypeHint`.

* Fix `class` keyword repetition in `UnrecognizedClass`.

* Mark stack as mutable.
jvican added a commit to jvican/pickling that referenced this pull request May 25, 2016
* Add a new unrecognized tag error.

* Abstract over `NoTypeHint`.

* Fix `class` keyword repetition in `UnrecognizedClass`.

* Mark stack as mutable.
jvican added a commit to jvican/pickling that referenced this pull request May 25, 2016
* Add a new unrecognized tag error.

* Abstract over `NoTypeHint`.

* Fix `class` keyword repetition in `UnrecognizedClass`.

* Mark stack as mutable.
jvican added a commit to jvican/pickling that referenced this pull request May 25, 2016
* Add a new unrecognized tag error.

* Abstract over `NoTypeHint`.

* Fix `class` keyword repetition in `UnrecognizedClass`.

* Mark stack as mutable.
jvican added a commit to jvican/pickling that referenced this pull request May 25, 2016
* Add a new unrecognized tag error.

* Abstract over `NoTypeHint`.

* Fix `class` keyword repetition in `UnrecognizedClass`.

* Mark stack as mutable.
jsuereth added a commit that referenced this pull request Jun 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants