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.

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

Add expected behavior for null values to decode functions #47

Merged
merged 7 commits into from Nov 24, 2018

Conversation

Projects
None yet
5 participants
@davezuch
Copy link
Contributor

davezuch commented Nov 15, 2018

What does this pull request do?

Renames decode combinators and introduces a new one that treats null values as missing values. See discussion here: #32

The reason for this boils down to the current combinators not behaving as most users would expect, but updating their behavior would break existing codebases without them knowing. So the solution was to rename the functions altogether, now matching their analogues in Haskell's Aeson library.

Also added some tests. I couldn't figure out how to test the decode combinators with QuickCheck. Even for the existing tests, the one that claimed to be testing .? (now .:) wasn't. So instead I added a suite of manual tests.

Where should the reviewer start?

src/Data/Argonaut/Decode/Combinators.purs contains the meat of the updates. Then the tests also illustrate the intended behavior.

How should this be manually tested?

Beyond running the tests, if you have any projects that use this library you could test those.

Other Notes:

This is a breaking change and will require a major version bump.

@thomashoneyman thomashoneyman requested a review from garyb Nov 15, 2018

@thomashoneyman thomashoneyman self-assigned this Nov 15, 2018

@thomashoneyman thomashoneyman removed the request for review from garyb Nov 15, 2018

@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Nov 15, 2018

I'm in favor of this change. The operators have the same functionality, but now match the highly-similar Aeson library more closely. There's now a more intuitive alternative to getFieldOptional, which has been a problem for me in the past and caused the issue you linked, #32. And I appreciate the tests and added documentation.

I'd like to leave this open for some time to let others affected by the change weigh in. I've cross-posted to Discourse to get a little more visibility as well.

@natefaubion

This comment has been minimized.

Copy link
Contributor

natefaubion commented Nov 15, 2018

Is there a reason to not also keep the old combinators, possibly marking them as deprecated with a warning?

@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Nov 15, 2018

@natefaubion Beyond the clutter, not really. I suppose they could be retained in the same module with a warning, or moved to another deprecated module (and then re-exported) for one major version.

Are you suggesting this simply to avoid breaking existing code?

@natefaubion

This comment has been minimized.

Copy link
Contributor

natefaubion commented Nov 15, 2018

Yes, if the only reason it is breaking is because we are removing a few functions, then I'd recommend just leaving them where they are and discouraging their use, possibly removing them with the next breaking compiler release.

@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Nov 16, 2018

@davezuch What about leaving the old function names (getField, etc.), introducing new operators (.:), adding the new function for .!?, and then putting warnings on the old operators? As we aren't changing the existing behavior, just adding a new function and changing the operators.

They could be moved to the bottom of the module and either just given a documentation warning or a Warn instance for compiler warnings. In the next breaking release of this library we can take them out.

@@ -24,28 +24,39 @@ Using [purescript-argonaut-core](https://github.com/purescript-contrib/purescrip

```purescript
someObject =

This comment has been minimized.

@thomashoneyman

thomashoneyman Nov 16, 2018

Member

This part of the example could be updated to use the combinators to define an instance of EncodeJson for MyType as that's a more common way to build these than this explicit jsonSingletonObject and so on.

@davezuch

This comment has been minimized.

Copy link
Contributor

davezuch commented Nov 16, 2018

possibly marking them as deprecated with a warning?

@natefaubion are you suggesting something like the Warn typeclass (https://liamgoodacre.github.io/purescript/warnings/2017/01/17/purescript-warn-type-class.html) or is that a little much?

@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Nov 17, 2018

It's aggressive, but I don't mind adding the type class warning if we're planning to remove them altogether in a future version of the library. Better to see the warnings in your build output than to never see them at all until it actually switches IMO. If you've been using the library for a while, then you may not be looking at the docs very often anymore.

@davezuch

This comment has been minimized.

Copy link
Contributor

davezuch commented Nov 19, 2018

Okay, I've added them back. Had to alias the original functions so that I could add the Warn constraint to the old operators without affecting the new ones, so now the file's really cluttered. I don't think it's a big deal though

@garyb

This comment has been minimized.

Copy link
Member

garyb commented Nov 19, 2018

Something to consider here - renaming the functions, even if the operator names are preserved, is still a breaking change, since people could have been using the named variants.

@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Nov 19, 2018

@garyb I believe @davezuch did preserve the original names with their original implementations (but new operators) when reverting, and the old operators have the old implementations but under new names. In a way, folks will now be using 'new' functions when they use the old operators, but the implementations are exactly the same except with the new Warn constraint. Then, he added another function / operator on top to handle this null issue.

IMO that shouldn't constitute a breaking change even if technically it is true that the functions have been renamed, because existing code shouldn't break. I've pulled this down on a personal project and things seem OK; I know CitizenNet has a lot of code exercising all these operators and occasionally named variants; might be a useful mini-test to make sure that stuff doesn't break @davezuch

@davezuch

This comment has been minimized.

Copy link
Contributor

davezuch commented Nov 19, 2018

For anyone using the named functions, nothing will change. For those using the existing operators, now they'll get compiler warnings telling them to use the new operators. Does that constitute a breaking change? I don't think it should, though I can see the argument for it

@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Nov 19, 2018

I'd like to leave this open for another day or two, and then I think it's ready to merge as a minor release (non-breaking). However, if folks disagree, then I can hold off.

@chexxor

This comment has been minimized.

Copy link

chexxor commented Nov 20, 2018

Might be a good idea to note in the project readme that this is now meant to be closer to Aeson than Argonaut. I haven't used either of those other libraries but it's what I gather from reading the discussion so far. That note can be added as part of a different PR, though. I can add it to #48 if you'd rather not add more commits, no problem.

@chexxor

This comment has been minimized.

Copy link

chexxor commented Nov 20, 2018

Looks like this project moved towards Aeson a long time ago, actually: #12

I'll just add a note to the other issue about updating the readme. 👍

@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Nov 24, 2018

I'm merging now. Thanks for your work on this, @davezuch.

@thomashoneyman thomashoneyman merged commit eabe759 into purescript-contrib:master Nov 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment