You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2021/01/29 16:11:52 UTC
[GitHub] [avro] tom-j-irvine opened a new pull request #1070: AVRO-2633 - csharp include schema doc
tom-j-irvine opened a new pull request #1070:
URL: https://github.com/apache/avro/pull/1070
This PR preserves the schema "doc" attributes when serializing the schema to json. The existing code would parse and include the documentation, but it wasn't passed along when serializing the schema.
### Jira
Jira Issue AVRO-2633 - C# - AvroGen tool - Document for record type is not included in the Schema field.
https://issues.apache.org/jira/projects/AVRO/issues/AVRO-2633
This pull request makes preserves the schema documentation when serializing the schema.
### Tests
My PR adds a new test case to SchemaTests.TestBasic
My PR adjusts SchemaTests.testFullName to use a modified constructor
### Commits
- [X] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
C# XML comments were updated for all modified methods
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] tom-j-irvine commented on a change in pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
tom-j-irvine commented on a change in pull request #1070:
URL: https://github.com/apache/avro/pull/1070#discussion_r578050603
##########
File path: lang/csharp/src/apache/test/Schema/SchemaTests.cs
##########
@@ -69,8 +69,12 @@ public class SchemaTests
typeof(SchemaParseException), Description = "No fields")]
[TestCase("{\"type\":\"record\",\"name\":\"LongList\", \"fields\": \"hi\"}",
typeof(SchemaParseException), Description = "Fields not an array")]
- [TestCase("[{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns1\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}," +
+ [TestCase("[{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns1\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}," +
"{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns2\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}]")]
+
+ // Doc
+ [TestCase("{\"type\": \"record\",\"name\": \"Test\",\"doc\": \"Test Doc\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}")]
+
Review comment:
Excellent point. I updated the test to use your round trip logic. As far as empty strings, my original code would have eliminated them from the output, but since the test was written expecting them, I changed it to preserve the doc attribute when specified; empty or not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] tom-j-irvine commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
tom-j-irvine commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-771152951
Not sure why the interop is failing. All tests run successfully for me on my local system.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] filipeesch commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
filipeesch commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-822516060
Hi, do you have any idea when this PR will be merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] tom-j-irvine commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
tom-j-irvine commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-822579042
> Hi. Do you know if there's anything missing here, anything preventing this fix from being merged? This bug is causing us a lot of trouble. Thanks
I created the PR and would love to see it merged as well. As far as I know, it is all good to go. I haven't contributed here before, so I'm not sure how we raise the priority. Let me know if there is something else I can do to get it moving.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] dougolima edited a comment on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
dougolima edited a comment on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-822517666
Yes, we are facing the same problem here. Our schemas were created with the 'doc' attribute in live environment and, for now, the only solution has been to create a new version of them without this attribute.
It would be great to have this fixed in master to avoid this workaround.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-935958421
Thanks again for the contribution. This should appear in release 1.11.0, and we're starting the release process soon!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] dougolima commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
dougolima commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-822517666
Yes, we are facing the same problem here. Our schemas were created with the 'doc' attribute in live environment and, for now, the only solution has been to create a new version of them without this attribute.
It would be great to have this fix in master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba merged pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
RyanSkraba merged pull request #1070:
URL: https://github.com/apache/avro/pull/1070
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] BarryDahlberg commented on a change in pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
BarryDahlberg commented on a change in pull request #1070:
URL: https://github.com/apache/avro/pull/1070#discussion_r577943118
##########
File path: lang/csharp/src/apache/test/Schema/SchemaTests.cs
##########
@@ -69,8 +69,12 @@ public class SchemaTests
typeof(SchemaParseException), Description = "No fields")]
[TestCase("{\"type\":\"record\",\"name\":\"LongList\", \"fields\": \"hi\"}",
typeof(SchemaParseException), Description = "Fields not an array")]
- [TestCase("[{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns1\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}," +
+ [TestCase("[{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns1\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}," +
"{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns2\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}]")]
+
+ // Doc
+ [TestCase("{\"type\": \"record\",\"name\": \"Test\",\"doc\": \"Test Doc\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}")]
+
Review comment:
There are a few doc tests in this file already like `TestRecordDoc` and `TestEnumDoc`. These check that the field is parsed in without error, which is all that this case does as well. None of them actually check that the doc string is written when serialising though.
Changing these existing tests to do something like write the schema out and parse it again will give you test cases for the whole cycle which I think is quite important to this PR, something like:
```
public void TestRecordDoc(string s, string expectedDoc)
{
var parsed = Schema.Parse(s) as RecordSchema;
Assert.IsNotNull(parsed);
Assert.AreEqual(expectedDoc, parsed.Documentation);
var roundTrip = Schema.Parse(parsed.ToString()) as RecordSchema;
Assert.IsNotNull(roundTrip);
Assert.AreEqual(expectedDoc, roundTrip.Documentation);
}
```
I'm not clear on how empty strings should be treated though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] tom-j-irvine commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
tom-j-irvine commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-780610440
> > Not sure why the interop is failing. All tests run successfully for me on my local system.
>
> Tests running OK here too. The test run looks like it failed on the step titled:
>
> > Install Java Avro for Interop Test
>
> It doesn't look like the error is related to this PR, perhaps try a rebase to latest and push again to try another test run?
Thanks - that seems to have worked. I hope I did it correctly, but it seems my changes are now against the latest code and checks are passing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] BarryDahlberg commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
BarryDahlberg commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-780237310
> Not sure why the interop is failing. All tests run successfully for me on my local system.
Tests running OK here too. The test run looks like it failed on the step titled:
> Install Java Avro for Interop Test
It doesn't look like the error is related to this PR, perhaps try a rebase to latest and push again to try another test run?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-822609714
Hello -- thanks for the contribution and your patience! I set the Fix version to 1.11.0 (and assigned you the JIRA) so it'll be on the radar! With my rusty C#, this looks like the right fix for this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] zehenrique commented on pull request #1070: AVRO-2633 - csharp include schema doc
Posted by GitBox <gi...@apache.org>.
zehenrique commented on pull request #1070:
URL: https://github.com/apache/avro/pull/1070#issuecomment-821896317
Hi. Do you know if there's anything missing here, anything preventing this fix from being merged? This bug is causing us a lot of trouble. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org