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

added documentation template #299

Merged
merged 4 commits into from
Jan 22, 2021

Conversation

mdumandag
Copy link
Contributor

@mdumandag mdumandag commented Jan 21, 2020

@mdumandag mdumandag added the 2.x client protocol 2.x related label Jan 21, 2020
@mdumandag mdumandag added this to the 2.0 milestone Jan 21, 2020
@mdumandag mdumandag self-assigned this Jan 21, 2020
@mdumandag mdumandag force-pushed the documentation-generator branch from d4a019d to daa18ef Compare January 31, 2020 14:43
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Apr 5, 2020

CLA assistant check
All committers have signed the CLA.

@mdumandag mdumandag force-pushed the documentation-generator branch from daa18ef to 0a352af Compare April 14, 2020 08:15
@mdumandag
Copy link
Contributor Author

Here is the sample of the generated markdown document. I think it can be reviewed from there easily.

https://gist.github.com/mdumandag/85dbfdaea7a8f57151bc90565126ba7d

@sancar
Copy link
Contributor

sancar commented Apr 14, 2020

@mdumandag
Copy link
Contributor Author

@sancar It is because of this codec documentation. https://github.com/hazelcast/hazelcast-client-protocol/blob/master/protocol-definitions/Client.yaml#L519 . Do you think that this level of detail is needed for this codec ?

@sancar
Copy link
Contributor

sancar commented Apr 14, 2020

Are we describing what Data is. For map put , it says Data as the type of the key and value.
May be we can put byte[] or, array of bytes instead of Data

Also I see Map_String_String on the types. It seems it is missed. Array of String to String mappings ?

@sancar
Copy link
Contributor

sancar commented Apr 14, 2020

I think the documentation is valuable for the ClientStatistics.
I can suggest using enclosing the docs with ``` to prevent them being processed.

@asimarslan asimarslan modified the milestones: 2.0, 2.1 Sep 3, 2020
@mdumandag mdumandag force-pushed the documentation-generator branch 2 times, most recently from 877dedf to 66ff26f Compare January 6, 2021 14:53
@mdumandag mdumandag force-pushed the documentation-generator branch 3 times, most recently from c8052a0 to b8be733 Compare January 18, 2021 07:47
@mdumandag mdumandag force-pushed the documentation-generator branch from b8be733 to 84dfb7e Compare January 18, 2021 07:47
@puzpuzpuz puzpuzpuz self-requested a review January 18, 2021 12:05
md/documentation-template.j2 Outdated Show resolved Hide resolved
{% endfor %}
{% endif %}
{% endfor %}
{% for service in services %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to mark some services as internal ones and exclude them from the documentation? I'm primarily talking of MC service.

Copy link
Contributor Author

@mdumandag mdumandag Jan 19, 2021

Choose a reason for hiding this comment

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

I am not really sure about this. I agree that it makes some sense to hide them but on the other hand, they are publicly defined and we don't have such separation in other parts of the protocol.

I am thinking of leaving the things as it is but I can quickly change the template to ignore these if we decide to hide them.

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discuss MC service primarily, it makes sense to hear from @emre. Emre are you fine with having MC-related client messages in the generated protocol documentation?

md/documentation-template.j2 Outdated Show resolved Hide resolved
md/documentation-template.j2 Outdated Show resolved Hide resolved
md/documentation-template.j2 Outdated Show resolved Hide resolved
md/documentation-template.j2 Outdated Show resolved Hide resolved
md/documentation-template.j2 Outdated Show resolved Hide resolved
md/documentation-template.j2 Outdated Show resolved Hide resolved
md/documentation-template.j2 Show resolved Hide resolved
md/documentation-template.j2 Show resolved Hide resolved
@mdumandag mdumandag force-pushed the documentation-generator branch from 58694c8 to 6d51479 Compare January 19, 2021 11:52
@mdumandag
Copy link
Contributor Author

Thanks a lot for the detailed review Andrey, I have resolved almost all of them apart from two. Could you take a look at them? Also, added Data and ByteArray to protocol data types (section 3) since we have String defined there. I believe we should mention those since all 3 of them are special, single frame types @puzpuzpuz

@puzpuzpuz
Copy link
Contributor

@mdumandag changes look good to me. Thanks!

As for the second one, if we decide that we should hide internal services from the doc, we could address it in a subsequent PR.

@mdumandag
Copy link
Contributor Author

Thanks a lot for the reviews guys. I am merging this as it is (with hiding internal services like MC). If there will be any objections to that, we can revisit this decision

@mdumandag mdumandag merged commit dc3b23c into hazelcast:master Jan 22, 2021
@mdumandag mdumandag deleted the documentation-generator branch January 22, 2021 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x client protocol 2.x related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants