search

datamapper / dm-types

DataMapper plugin providing extra data types
http://datamapper.org/
MIT License
55 stars 80 forks source link

Relax unneeded json dependency #62

Closed ujifgc closed 11 years ago

dkubb commented 11 years ago

If the json dependency is removed, how would the automated tests know to install json to test the DataMapper::Property::Json type? Unless it's a dependency of some other gem and I'm missing it, but I'm not sure that's the case.

postmodern commented 11 years ago

The json dependency should probably be a development dependency?

dkubb commented 11 years ago

@postmodern well, it is needed for the type to actually be usable. There is a chance that json will be pulled in as another gem's dependency, but we can't rely on that. (EDIT: I mean in the context of an application)

I'm not a huge fan of distributing a gem which has dependencies that would not be discovered until you actually tried to use it. What is the main reason for wanting to remove the json dependency?

ujifgc commented 11 years ago

json gem is absolutely not needed in any environment. We include MultiJson gem and use it for production load/dump and for tests. This MultiJson gem resolves everything related to it's functions.

By the way, dm-types edge (1.3) has it already removed from .gemspec.

solnic commented 11 years ago

Yeah on master json is already removed. Unfortunately I don't remember why it's still in release-1.2 branch. I think it's OK to remove it.

@dkubb we switched from hard dependency on json in favor of multi_json so that people can choose which json backend they want to use. This is a better approach given we deal with multiple VMs with different JSON backends.

EDIT: I think my motivation behind leaving it in release-1.2 was to keep backwards compatibility. If we removed it from 1.2 release some people might end up with broken system due to a missing require 'json'.

ujifgc commented 11 years ago

Okay, thanks.

dkubb commented 11 years ago

I didn't realize multijson has a vendored json implementation that it falls back to when a better one is not available. That changes my opinion on what we should do.

@solnic I think if people are relying on transitive dependencies and not specifying what they use explicitly that's a bug in their system and we don't have to factor that into our decision.

Given the new information I have about multijson, I would support removing json as an explicit dependency, unless there is a behaviour the json gem provides to DM that is different from multijson. Can anyone think of an issue this would cause?