search

ept / avrodoc

Documentation tool for Avro schemas
Apache License 2.0
148 stars 82 forks source link

Incorporating Inome, Inc changes #2

Closed inome closed 11 years ago

inome commented 11 years ago

Hi--

We have made a number of changes that you may be interested in:

Let me know if you have any questions. We are currently running our avrodoc version in our build environment successfully.

Thank you, Micah Software Engineer :: Inome, Inc.

ept commented 11 years ago

Thanks Micah! Appreciate your contribution, and I will go through it in detail.

One request: could you add an example to the schemata directory that exercises the new messages feature? We don't use Avro RPC/IDL so I don't have an example of my own. I agree though that it would be good to support it.

inome commented 11 years ago

Hi @ept --

I just added the two commits above. The one (8a418f7) contains an example avsc file that has a single message in it ('ping'). It also contains a newer type of object called an 'error' that is available in the latest version of avro schema.

I forgot to push out the changes I made for supporting the error type hence the new commit ffe5ea1.

As far as the syntax, I like your suggestion for supporting a variety of styles on the command line. I went with what I did as it suited our immediate needs but I think in the broader case, your suggestion makes a lot of sense.

Let us know if you have any questions on this. I hope that you will find these changes useful.

Thank you, Micah Software Engineer :: Inome, Inc.

ept commented 11 years ago

Excellent, thanks! The example is useful. I'd like to iterate on the presentation of protocols/messages a bit before merging, which I'm doing on this branch. I've cherry-picked just the commits for adding protocol/message support, as the command-line argument parsing is a separate concern.

I'll play with it a bit and will circle back to get your feedback later :)

inome commented 11 years ago

I like your proposition for removing the output for single file. We only had the need for a single file hence us removing it for our use case but for the general population, you are correct that this information is likely useful.

On a side note, we may be converting this GitHub account to an 'organization' today but would still like to get credit for the commits. Do you have a problem if I re-upload the commits under one of the 'members' profiles this afternoon and then you re-pick them? I'm hoping to get this completed this morning.

Micah

mphuff commented 11 years ago

Hi @ept --

Looks like you can ignore my comment about re-incorporating changes. Turns out that the verbiage GitHub uses to indicate "bad things will happen when you convert this account to an organization" were a little over the top :)

You can also ignore my testing commits.

Let me know if you have any other questions.

Micah

ept commented 11 years ago

Cool. Git maintains the authorship information for commits, so you'll be credited, no worries :)

mphuff commented 11 years ago

Hi @ept --

Just checking in to see how things are coming along with merging these changes in? We're about to deploy my version of your code out in to our production environment but I'd rather use yours if it is ready so that we can more easily get future updates.

Thanks, Micah

ept commented 11 years ago

Hi Micah, sorry for the delay. I've not forgotten, just got quite a few things on my plate at the moment. I hope get to work on this at the weekend. You should be able to easily switch back to the upstream version when it does what you need.

ept commented 11 years ago

Hey Micah, could you try the version on the protocols branch and let me know if it works for your schemas? Any UI functionality that's missing or is confusing when you feed in your schemas?

I've made a new sidebar section for protocols, and grouped all messages from a protocol under the protocol name. I thought it would be clearer that way:

Screenshot of design for protocols

I also fixed a few bugs, e.g. it now allows types to be defined inline in a message as well as in the separate types section (both are legal, if I read the Avro spec correctly).

mphuff commented 11 years ago

Those changes look great! I ran them against one of our more complicated schemas and everything generated perfectly. The only comment I have about them would be to maybe not show the 'errors' section on a message if there are no errors defined for that particular message.

We've been reading through the schema more closely lately as we finalize some of our code generators and came across the following couple of elements in Avro schema 1.7.3 that I don't see accounted for as of yet in the code (and we didn't make because we didn't need them). I'm not sure if you want to implement something for them but it may be worth it to be 'fully compatible' with Avro 1.7.3:

Let me know if you'd like help with either of these. We have an interest in getting this project fully compatible with the above mentioned pieces so if you want help, let us know and I can branch off of your protocols branch and open a new pull request.

Thank you for your help with this project!

Micah

ept commented 11 years ago

Cool, thanks for testing. I'll merge the protocols branch into master and close this pull request, before it gets too sprawling.

I'd love it if you could add support for oneway and ordering, and omit the errors section if it's empty, on a new pull request. Another thing I forgot is default values for fields — could you add that too?

I also opened #4 for supporting the output file command line argument that you need; either of us can work on that when we have time.

Thanks for your help!

mphuff commented 11 years ago

Can you update the deployed bits on the node package manager website? https://npmjs.org/package/avrodoc appears to be out of date in that if I run "npm install -g avrodoc" my changes are not incorporated.