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

External tag #316

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

External tag #316

wants to merge 3 commits into from

Conversation

Sprexatura
Copy link

@Sprexatura Sprexatura commented Aug 17, 2018

Adopt external-tag to serialized union values

@Sprexatura Sprexatura changed the base branch from 0.4-maintenance to master August 17, 2018 07:31
@kanghyojun
Copy link
Member

As I mentioned earlier, there are some style issues.

src/Nirum/Targets/Python.hs:850:1: too long line (107 chars)
src/Nirum/Targets/Python.hs:850:108: trailing (1) white space
src/Nirum/Targets/Python.hs:875:1: too long line (92 chars)
src/Nirum/Targets/Python.hs:876:57: trailing (1) white space
src/Nirum/Targets/Python.hs:884:15: trailing (4) white space

@kanghyojun
Copy link
Member

kanghyojun commented Aug 17, 2018

@dahlia @Kroisse Could you all review the idea of this PR regardless of the failing of CI? We decided to support external tag by adding @external-tag annotation on record type. And maybe in the next minor version updates, we could support external tag as default.

Copy link
Member

@dahlia dahlia left a comment

Choose a reason for hiding this comment

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

Annotations available to (virtually) every target should be documented in docs/annotation.md file as well (besides docs/serialization.md file).

@@ -10,6 +10,12 @@ To be released.

- The `uri` type has completly gone; use `url` instead.
[[#126], [#281] by Jonghun Park]
- Support [external_tag][etag] via '@external-tag' annotation.
Copy link
Member

Choose a reason for hiding this comment

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

Changelogs should be the past tense. Also the following explanation does not have enough information:

external_tag via '@external-tag' annotation

@@ -250,6 +250,27 @@ It's represented in JSON to:
In a similar way to a record type, undefined fields in a payload are ignored
by deserializer.

If '@external-tag' annotation is given on tag below example
Copy link
Member

Choose a reason for hiding this comment

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

A trailing colon is missing.

@@ -37,7 +37,8 @@ union shape
point upper-left,
point lower-right
)
| circle (point origin, offset radius)
| @external-tag
Copy link
Member

Choose a reason for hiding this comment

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

Should the @external-tag annotation be applied to tags, not union types? Does that mean some tags could be serialized as external-tag-style while some other tags are still serialized with "_tag" field at a time?

{
"east-asian-name": {
"_type": "name",
"_tag": "east-asian-name",
Copy link
Contributor

Choose a reason for hiding this comment

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

This field might be not necessary, because it already presented above.

Copy link
Contributor

Choose a reason for hiding this comment

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

And, I think @external-tag should be associated on the whole union, not on its tag. If not, it loses the advantage that can be decide which tag should be parsed at once.

@codecov
Copy link

codecov bot commented Aug 20, 2018

Codecov Report

Merging #316 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   76.22%   76.26%   +0.04%     
==========================================
  Files          34       34              
  Lines        2902     2903       +1     
  Branches      194      196       +2     
==========================================
+ Hits         2212     2214       +2     
+ Misses        496      493       -3     
- Partials      194      196       +2
Impacted Files Coverage Δ
src/Nirum/Targets/Python.hs 88.2% <100%> (+0.03%) ⬆️
src/Nirum/Constructs/Docs.hs 94.73% <0%> (ø) ⬆️
src/Nirum/Constructs/Annotation/Internal.hs 77.27% <0%> (ø) ⬆️
src/Nirum/Constructs/Identifier.hs 98.14% <0%> (+1.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c405f6...21f7c99. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants