search

eiffelhub / json

Eiffel JSON library
Other
18 stars 13 forks source link

API and library improvements #10

Closed Conaclos closed 7 years ago

Conaclos commented 10 years ago

Overview

This pull-request prepare the next. A final goal is to allow to use Contextual typing (see https://docs.eiffel.com/sites/docs.eiffel.com/files/expression_language_0.pdf) when they will be available. i.e. object (a_json: JSON_VALUE; a_type: TYPE [?]): ?

jvelilla commented 10 years ago

The changes LGTM, but I'm worried because some of them will break existing code. So, maybe is time to create a new JSON library.

oligot commented 10 years ago

Looks like there is a lot happening in this PR. Could you try to split it in multiple PR to ease the review process ?

jocelyn commented 10 years ago

I reviewed mainly the core, and part of the converter (I did not reviewed the tests classes) But for me, I would not agree to merge this pull request MAINLY because this is breaking existing code. Please use "obsolete" , so that you can do most of your refactorying, but without breaking existing code.

As Javier suggested, maybe this would be time to create a new version as JSON v2 , but first , let's try to use to have your changes integrated with smart usage of obsolete functionality.

GOAL= keep backward compatibility + improve existing library with your suggestions.

Conaclos commented 10 years ago

I took the freedom to introduce breaking change for a simple reason: the library status is "unstable", meaning that its interfaces can change. However, now I know that several persons already use this library. Therefore I am agree with the idea to keep backward compatibility.

Conaclos commented 10 years ago

@oligot I don't know if it is possible in the current state to divide this PR. I take a look for it.

I take note for next ones. For this one, the best way is certainly to review each commits.

oligot commented 10 years ago

As there are 6 items in the overview, you could create a pull request for each one. Anyway, this is just a suggestion for the next ones as @jocelyn and @jvelilla have already reviewed it.

Conaclos commented 10 years ago

I restored old features.

Only converters are not backward-compatible...

jocelyn commented 10 years ago

For info, I created a new pull request https://github.com/eiffelhub/json/pull/11 that may address part of this pull request 10. (I must say I forgot about that one). On Converter, I would suggest to create a new library using the json library.

jocelyn commented 7 years ago

Close as it is now quite old. If you think we should review this patch, and integrate part of it, please re-open it. Thanks.