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.

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

Fix model_json and nitweb #2327

Merged
merged 3 commits into from
Dec 3, 2016
Merged

Fix model_json and nitweb #2327

merged 3 commits into from
Dec 3, 2016

Conversation

Morriar
Copy link
Member

@Morriar Morriar commented Nov 4, 2016

Was broken since #2312

  • Migrate model_json to new serialization API
  • Add tests to model_json to ensure this doesn't happen anymore
  • Migrate web package and nitweb to new API

@xymus a lot of serialization for you. Note: for each entity, I needed two deserialization version full and no full. Wasn't sure on how to handle this concern properly with your API

return array
end

redef class JsonSerializer
Copy link
Contributor

Choose a reason for hiding this comment

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

To answer your question about the 2 serialization versions, the serialization API is designed for the clients to subclass the engines to customize their behavior. Here you should be able to create a subclass of JsonSerializer instead of a redef (I'd name the subclass FullModelSerializer). You can then use the distinct class (which is also the visitor) to change the serialization behavior without adding methods. You would not need the custom serialize_full, core_serialize_full and accept_full_json_serializer, instead you can use type checks in core_serialize_to:

class FullModelSerializer
    super JsonSerializer
end

redef class MPropDef
    redef fun core_serialize_to(v) do
        super
        v.serialize_attribute("is_intro", is_intro)

        if v isa FullModelSerializer then
            v.serialize_attribute("mclassdef", to_mentity_ref(mclassdef))
            v.serialize_attribute("mproperty", to_mentity_ref(mproperty))
            v.serialize_attribute("intro", to_mentity_ref(mproperty.intro))
            v.serialize_attribute("intro_mclassdef", to_mentity_ref(mproperty.intro.mclassdef))
            v.serialize_attribute("mmodule", to_mentity_ref(mclassdef.mmodule))
            v.serialize_attribute("mgroup", to_mentity_ref(mclassdef.mmodule.mgroup))
            v.serialize_attribute("mpackage", to_mentity_ref(mclassdef.mmodule.mpackage))
        end
    end
end

This would reduce code duplication and you could remove the intrude importation. The class could also have attributes to further customize the serialization of the model for different usage. And this would even be compatible with a binary engine by simply sharing a common interface!

v.serialize_attribute("mdoc", mdoc_or_fallback)
v.serialize_attribute("visibility", visibility.to_s)
v.serialize_attribute("modifiers", collect_modifiers)
v.serialize_attribute("location", location)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice clean up!

@Morriar
Copy link
Member Author

Morriar commented Nov 9, 2016

Reroll with @xymus suggestions on the two serialization versions

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

+1

@@ -355,16 +338,11 @@ end
redef class POSetElement[E]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note for the future, POSetElement defines serialization services in its introduction module. However, it writes the fat private attributes (tos, froms) including element, count & poset, instead of flattening the public getters (greaters, smallers) as done here, so the resulting JSON objects differs. This is not a problem, but the difference may cause surprises in the future.

Conflicts would arise if this module is compiled/joined with the compiler or interpreter, or if the deserialization services of POSetElements are changed. (see #2311) Until then, everything is fine.

@@ -26,7 +26,7 @@ module model_json

import model::model_collect
import json::static
import json
intrude import json::serialization_write
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still need the intrude?

@Morriar Morriar force-pushed the fix-nitweb branch 2 times, most recently from f8b7846 to 4052fdb Compare November 18, 2016 16:30
@Morriar Morriar force-pushed the fix-nitweb branch 2 times, most recently from cdc2fbe to 75234c2 Compare November 23, 2016 21:32
privat added a commit that referenced this pull request Nov 24, 2016
Was broken since #2312

* Migrate `model_json` to new serialization API
* Add tests to `model_json` to ensure this doesn't happen anymore
* Migrate `web` package and `nitweb` to new API

@xymus a lot of serialization for you. Note: for each entity, I needed two deserialization version `full` and no full. Wasn't sure on how to handle this concern properly with your API

Pull-Request: #2327
Reviewed-by: Alexis Laferrière <[email protected]>
@Morriar
Copy link
Member Author

Morriar commented Nov 29, 2016

test this please

privat added a commit that referenced this pull request Dec 2, 2016
When working with test suites, one can now use `NIT_TESTING_PATH` to retrieve the test suite path.

It can be used to access files based on the current test suite location:

~~~nit
class MyTest
	super TestSuite

    fun test_suite_path do
        assert "NIT_TESTING_PATH".environ.basename == "my_test_suite.nit"
    end
end
~~~

Useful for test suites based on model loading like in #2327.

Pull-Request: #2331
privat added a commit that referenced this pull request Dec 3, 2016
Was broken since #2312

* Migrate `model_json` to new serialization API
* Add tests to `model_json` to ensure this doesn't happen anymore
* Migrate `web` package and `nitweb` to new API

@xymus a lot of serialization for you. Note: for each entity, I needed two deserialization version `full` and no full. Wasn't sure on how to handle this concern properly with your API

Pull-Request: #2327
Reviewed-by: Alexis Laferrière <[email protected]>
@privat privat merged commit a1547db into nitlang:master Dec 3, 2016
privat added a commit that referenced this pull request Dec 3, 2016
Was broken since #2312

* Migrate `model_json` to new serialization API
* Add tests to `model_json` to ensure this doesn't happen anymore
* Migrate `web` package and `nitweb` to new API

@xymus a lot of serialization for you. Note: for each entity, I needed two deserialization version `full` and no full. Wasn't sure on how to handle this concern properly with your API

Pull-Request: #2327
Reviewed-by: Alexis Laferrière <[email protected]>
@Morriar Morriar deleted the fix-nitweb branch January 22, 2018 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants