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 generated JsonDesererialzer Java code errors #75

Merged
merged 2 commits into from
Feb 14, 2018

Conversation

juliaogris
Copy link
Contributor

Fixes #66.

Changes proposed in this pull request:

  • Fix bad case in serializer.py
  • Add a Java test

@anz-bank/sysl-developers

@codecov
Copy link

codecov bot commented Feb 14, 2018

Codecov Report

Merging #75 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #75   +/-   ##
=======================================
  Coverage   57.39%   57.39%           
=======================================
  Files          36       36           
  Lines        7037     7037           
=======================================
  Hits         4039     4039           
  Misses       2998     2998
Impacted Files Coverage Δ
src/sysl/exporters/json_out/serializer.py 28.82% <ø> (ø) ⬆️

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 a41ecdc...3c05295. Read the comment docs.

Copy link
Contributor

@camscale camscale left a comment

Choose a reason for hiding this comment

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

LGTM with whitespace changes.

BTW, this was made hard to review because I had to reverse engineer the issue. The PR and commit do not reference the issue, though I found it from the test article.sysl file. The issue didn't really explain the problem and what the solution should be. The function java.CamelCase just capitalises the first letter of the word, which is not camel casing (being camel case says nothing about the case of the first letter, just how words are joined into a single symbol).

In the end, I figured out the nature of the problem so I can see how the code you wrote addresses it, but that info should be recorded somewhere.

}

public static Article articleFromJson() throws IOException {
JsonFactory factory = new JsonFactory();
Copy link
Contributor

@camscale camscale Feb 14, 2018

Choose a reason for hiding this comment

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

The indenting has gone funny in this method - tabs vs spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks. I run into this quite regularly - I think it's when I copypasta from somewhere else. Do you have a tool that you run to avoid such issues or should I just see if I can change something in my editor?

public static Article articleFromJson() throws IOException {
JsonFactory factory = new JsonFactory();
String s = "{\"content\" : { \"Text\" : \"Peace and love\" } }";
JsonParser p = factory.createParser(s);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's a double space before the =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@camscale
Copy link
Contributor

So I was wrong about the PR not referencing the the issue - there it is right up front. I guess I missed it because I go straight to the commits. I'll pay more attention next time.

@juliaogris juliaogris merged commit 8feb5bb into anz-bank:master Feb 14, 2018
@juliaogris juliaogris deleted the fix-gen-deserealizer branch February 14, 2018 10:26
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.

3 participants